btrfs-progs pull request for coverity fixes

btrfs-progs pull request for coverity fixes

[Zach Brown]

Hi gang,

Eric and I went through the warnings that a Coverity scan raised against
the reasonably recent btrfs-progs that's in Fedora.  We tried to tackle
the lowest hanging fruit: these are the most obvious and least risky
fixes.

We got up to 40 patches before running out of steam.  I was going to
bomb the list with them but figured I'd first try not to annoy everyone
and just send this pull request.  I can send all the patches if people
want them in the archives, etc.

I've only compile tested these.  What do people use for btrfs-progs unit
tests?

Most of the bugs fixed are tiny and boring: leaking fds and memory,
using closed or uninitialized fds, clobbering paths with long arguments,
that sort of thing.

But there are two interesting bugs worth mentioning:

 7d365c5 btrfs-progs: don't write memory after sb to disk
 9e4ad99 btrfs-progs: use ftw() unstead of system("du")

The first is more of a curiousity.  There's nothing valuable in the mkfs
heap that can get leaked into the unused end of the super block, but
it's worth fixing.  The second is a real piece of work.  It's good that
it was hiding off in mkfs in some weird option.

David, I rebased these on the integration branch because.. well, it was
easy and seemed like where they'll end up.  There were only a handful of
things to fix when rebasing from Chris' current master.

Anyway, holler if we've done anything dim.  I'm happy to clean these up
and resend as needed.

- z

The following changes since commit 2161e1b6f35d1c084fda49b479951219117c86e9:

  Btrfs-progs: use btrfs_lookup_first_block_group when fixing accounting (2013-02-01 17:56:42 +0100)

are available in the git repository at:

  http://git.zabbo.net/cgit/btrfs-progs.git cov-fixes-v1-integration-20130201

for you to fetch changes up to 2986545ccd655273658e0e4463a669bb1893ba68:

  btrfs-progs: initialize pipefd[] for error path (2013-02-05 16:09:41 -0800)

----------------------------------------------------------------
Eric Sandeen (9):
      btrfs-progs: don't double-close prg_fd
      btrfs-progs: don't use closed fd
      btrfs-progs: zero out inspect ioctl args
      btrfs-progs: fix mdresotre typo in function names
      btrfs-progs: remove duplicate __setup_root
      btrfs-progs: fix name lengths in cmd_subvol_create
      btrfs-progs: simplify ioctl name copy and null termination
      btrfs-progs: fix overflows of ioctl name args
      btrfs-progs: initialize pipefd[] for error path

Zach Brown (31):
      btrfs-progs: treat super.magic as an le64
      btrfs-progs: return error from commit_tree_roots()
      btrfs-progs: more carefully check eb backrefs
      btrfs-progs: use ftw() unstead of system("du")
      btrfs-progs: remove unused info_fd
      btrfs-progs: fix copy-n-paste error checking
      btrfs-progs: remove dead code that checks null eb
      btrfs-progs: don't free null path
      btrfs-progs: break after printing FREE_INO
      btrfs-progs: don't close(-1)
      btrfs-progs: don't return -EBUSY from main()
      btrfs-progs: don't close(<0) in subvol create
      btrfs-progs: check for open failure, don't close
      btrfs-progs: impossible BUG_ON meant to test empty
      btrfs-progs: don't write memory after sb to disk
      btrfs-progs: array indexes must be < ARRAY_SIZE()
      btrfs-progs: free path on read_chunk_tree error
      btrfs-progs: fix overflow in btrfs_scan_one_dir()
      btrfs-progs: don't leak in set_extent_bits
      btrfs-progs: fix scrub socket leak
      btrfs-progs: scrub can leak fd 0
      btrfs-progs: remove unused arguments
      btrfs-progs: free bits in check_extents()
      btrfs-progs: close fd in qgroup show
      btrfs-progs: free path before returning
      btrfs-progs: don't leak fd in resize
      btrfs-progs: close ioctl fd in find new
      btrfs-progs: don't leak inherit on errors
      btrfs-progs: don't leak multi-bio in find_root()
      btrfs-progs: close fd in inode resolve
      btrfs-progs: don't leak fds in logical resolve

 btrfs-image.c      |  8 ++++----
 btrfs-show-super.c |  2 +-
 btrfs-vol.c        |  2 +-
 btrfsck.c          | 28 +++++++++++++++-------------
 btrfsctl.c         |  7 +++----
 cmds-device.c      | 12 ++++--------
 cmds-filesystem.c  | 14 +++++++-------
 cmds-inspect.c     | 20 ++++++++++++++++----
 cmds-qgroup.c      |  3 +--
 cmds-receive.c     |  8 +++-----
 cmds-scrub.c       | 14 +++++++++-----
 cmds-send.c        |  6 +++---
 cmds-subvolume.c   | 19 ++++++++-----------
 ctree.c            |  8 +-------
 ctree.h            |  2 +-
 disk-io.c          | 27 ++++++++++++++++++---------
 disk-io.h          |  4 ++++
 extent_io.c        |  8 +++++---
 find-root.c        | 27 +--------------------------
 kerncompat.h       | 10 ++++++++++
 mkfs.c             | 47 +++++++++++++++++++++++++++--------------------
 print-tree.c       |  1 +
 restore.c          | 10 ++++------
 utils.c            | 37 +++++++++++++++++++++++++++----------
 utils.h            |  5 +++++
 volumes.c          |  4 ++--
 26 files changed, 181 insertions(+), 152 deletions(-)

Re: btrfs-progs pull request for coverity fixes

[Gene Czarcinski]

On 02/05/2013 07:49 PM, Zach Brown wrote:
> Hi gang,
>
> Eric and I went through the warnings that a Coverity scan raised against
> the reasonably recent btrfs-progs that's in Fedora.  We tried to tackle
> the lowest hanging fruit: these are the most obvious and least risky
> fixes.
>
If you ran your tests against the btrfs-progs in Fedora 18 then your 
tests may not have found problems fixed by the "valgrind" patch on F18.  
This patch is not applied to the current David Sterba's integration 
branches.

If you have the time, it might be useful (my opinion) to run your tests 
against Sterba's integration-02130201 branch ... some of the problems 
may be fixed and other not but you also may find some additional 
problems.  IMO, this branch (or something similar) will be the basis for 
a future v0.20 btrfs-progs ... at least that is what I am using/testing 
with.  There are over 80 commits in this branch over what is the baseis 
for the package in Fedora 18.

Gene

Re: btrfs-progs pull request for coverity fixes

[Eric Sandeen]

On 2/6/13 8:27 AM, Gene Czarcinski wrote:
> On 02/05/2013 07:49 PM, Zach Brown wrote:
>> Hi gang,
>> 
>> Eric and I went through the warnings that a Coverity scan raised
>> against the reasonably recent btrfs-progs that's in Fedora.  We
>> tried to tackle the lowest hanging fruit: these are the most
>> obvious and least risky fixes.
>> 
> If you ran your tests against the btrfs-progs in Fedora 18 then your
> tests may not have found problems fixed by the "valgrind" patch on
> F18.  This patch is not applied to the current David Sterba's
> integration branches.

The original one was done against F18 with the valgrind patch
in place, yes.  So it may have missed several things.  I can easily
do the same analysis against any codebase if/when it's appropriate.

> If you have the time, it might be useful (my opinion) to run your
> tests against Sterba's integration-02130201 branch ... some of the
> problems may be fixed and other not but you also may find some
> additional problems.  IMO, this branch (or something similar) will be
> the basis for a future v0.20 btrfs-progs ... at least that is what I
> am using/testing with.  There are over 80 commits in this branch over
> what is the baseis for the package in Fedora 18.

I agree that we need to re-run against all those patches, but I think
I will wait until they make it all the way upstream.

Until it gets to the point where development and patch
integration happens upstream (or at least in an orderly, predictable
manner), and we have timely releases of userspace for distro consumption,
it's going to be a little hit and miss with tools like this, since
there are so many different codebases out there right now, with distros
all maintaining their own large patchsets.

There are certainly more static analysis issues to fix, but Zach
and I thought we'd just see if this group of patches managed to
make it upstream before we went a lot further with it.

Thanks,
-Eric

> Gene

Re: btrfs-progs pull request for coverity fixes

[Chris Mason]

On Tue, Feb 05, 2013 at 05:49:07PM -0700, Zach Brown wrote:
> Hi gang,
> 
> Eric and I went through the warnings that a Coverity scan raised against
> the reasonably recent btrfs-progs that's in Fedora.  We tried to tackle
> the lowest hanging fruit: these are the most obvious and least risky
> fixes.

Awesome, thanks Zach and Eric.  I've got this rebased on top of Dave's
integration pull from today, and I'm doing a bunch of tests.

-chris

Re: btrfs-progs pull request for coverity fixes

[Zach Brown]

> Awesome, thanks Zach and Eric.  I've got this rebased on top of Dave's
> integration pull from today, and I'm doing a bunch of tests.

Great, thanks for the pull.  We're happy to help!

- z