xfsdump cleaning patchset

xfsdump cleaning patchset

[Jan Tulak]

Hi guys

I did some xfsdump cleaning where I focused on getting rid of all the
memory leaks reported by Coverity. There is still a lot of things to
do for another time: issues by Coverity that were not fixed in this
set, sprintf -> snprintf because there is a plenty of places for
buffer overflow, etc...

The set has about 30 patches, so I won't spam the mailing list with
the first iteration of review and instead, I uploaded it here:
https://github.com/jtulak/xfsdump/tree/for-review


The changes:

- Whitespace fixes. After speaking with Eric and because nobody
touches xfsdump anyway, I run a few sed commands and tried to fix the
crazy indentation to kernel style. It was an automated replacement, so
there can be places where it didn't work as intended, but in overall,
it helps with readability. Other whitespace issues I tackled are
trailing whitespaces.

- A lot of free(ptr) and close(fd). Xfsdump did not clean on errors,
so most of these are added to return -ERR; statements. These
after-error leaks were not an issue on itself, but it really polluted
the Coverity output.

- Other changes like dead code removal, null dereference, or
enlargement of a too small buffer for snprintf.


The patches are organised in this way:

- Whitespace issues are in two patches (indentation and trailing spaces).
- Resource leaks are one patch per file, with Coverity issue IDs (CID)
mentioned in the commit message for issues reported by the web scan. I
also used a custom instance of Coverity that reported some other
leaks, and those do not have any CID. If that seems still too
cluttered, I can break it to one patch per function, but that might
produce too many commits with two or three changes.
- And the remaining changes, like a dead code removal, are one patch
per issue/CID


I tested it as I could, but xfstests coverage is not large and part of
the tests requires a tape I don't have anyway. I also tested it with
valgrind to see if any errors got introduced, and from this
comparison, it came out clean (as much as I can tell when valgrind
throws 2k errors on me both pre and post patch).


Have a look.

Cheers,
Jan
--
Jan Tulak

Re: xfsdump cleaning patchset

[Christoph Hellwig]

On Mon, Oct 22, 2018 at 07:26:17PM +0200, Jan Tulak wrote:
> Hi guys
> 
> I did some xfsdump cleaning where I focused on getting rid of all the
> memory leaks reported by Coverity. There is still a lot of things to
> do for another time: issues by Coverity that were not fixed in this
> set, sprintf -> snprintf because there is a plenty of places for
> buffer overflow, etc...
> 
> The set has about 30 patches, so I won't spam the mailing list with
> the first iteration of review and instead, I uploaded it here:
> https://github.com/jtulak/xfsdump/tree/for-review

Please send an actual patch series, github is completely unusable
for reviews..

Re: xfsdump cleaning patchset

[Eric Sandeen]

On 10/22/18 12:43 PM, Christoph Hellwig wrote:
> Please send an actual patch series, github is completely unusable

Sorry, I had asked Jan to put up a git tree first as a sanity
check before sending $UNKNOWN_NR patches to the list, I just
wanted to quickly get a sense of the work he'd done, and have
the full patch series sent if it looked like it was on the right
path.

So, my bad.

-Eric

Re: xfsdump cleaning patchset

[Eric Sandeen]

On 10/22/18 1:07 PM, Eric Sandeen wrote:
> On 10/22/18 12:43 PM, Christoph Hellwig wrote:
>> Please send an actual patch series, github is completely unusable
> 
> Sorry, I had asked Jan to put up a git tree first as a sanity
> check before sending $UNKNOWN_NR patches to the list, I just
> wanted to quickly get a sense of the work he'd done, and have
> the full patch series sent if it looked like it was on the right
> path.
> 
> So, my bad.
> 
> -Eric
> 

I mostly just wanted to see how it was broken up and the patch
style, which looks mostly ok.  But while we're here:

First, what testing was done on the patchset?

For Coverity issues, please use:

Addresses-Coverity-ID: XYZ

or even i.e.

Addresses-Coverity-ID: XYZ ("Missing break in switch")

in the body instead of putting the CID in the subject (which wasn't
done consistently in any case).  It'd be good to have the coverity
ID in each patch that addresses one; more than one CID mentioned
via several patch tags is fine.


I'd also kind of hoped to do more whitespace/style fixup before
making code changes but that'll make your later patches impossible
to apply, so maybe we'll just leave it for now.


I'm not a huge fan of "fix buffer overflows by making them arbitrarily
bigger" - can we use i.e. snprintf to make sure it won't happen again,
and maybe size them based on ... something, instead of "eh, [5000] is
hopefully enough?"

i.e. in 

df48737f  xfsdump: fix string truncation 

"cmd" gets:

snprintf(cmd, sizeof(cmd), "cp %s %s", stobjfile, dst_stobjfile);

Each of those is up to MAXPATHLEN, which (I think) is 4096,
so even your 5000 isn't enough in extreme cases.  OTOH you may not
want a 2*4096+5 array on the stack, so maybe it should be allocated.
Or, maybe system("cp A B") is crazy in the first place, don't we have
rename() for that purpose?

In the same file, 

-static char txt[256];
+static char txt[300];

we snprintf to txt in 3 places:

snprintf(txt, sizeof(txt), "path:  %s", invtentry->ie_filename);
ie_filename is 128 chars so this should be fine.

and 2 more similar to:
snprintf(txt, sizeof(txt), "start: %s", ctime32(&invtentry->ie_timeperiod.tp_start));

where "ctime32" calls ctime() which in general isn't going to be returning
anywhere near 250 bytes, so it's not clear why 300 is any "better."

similar for the change to:

#define MAX_UNAMECMD MAXHOSTLEN+40

there is no indication that "+100" is sufficient - rsh_path could again
be up to PATH_MAX.  For this kind of stuff don't guess, either figure out
the max, or gracefully detect an overflow and use error handling if it does,
or find a way to do it that doesn't require allocating 10k of buffer
on the stack.  ;)


>From spot-checking a few commits, please take another careful pass
through to look at context and code history to be sure that you're
really fixing the code properly and not just making coverity and/or
gcc be quiet; for example in

0ab032a8 common/drive_minrmt.c: remove a dead code (CID 1056289) 

you saw that coverity spotted dead code and so you removed it, but it
shouldn't be dead, it should be somewhere else, I think - look up about
30 lines from your change for a hint.  :)


For some of the resource leak fixes, you might consider using something
like "goto out_free;" rather than adding cleanup repeatedly at multiple
return points.  Sometimes that's cleaner.


TBH each of these is going to take careful review, nobody really
knows how xfsdump works anymore so we'll have to be very careful that
nothing breaks.

Anyway, give it another check through, make sure you understand the
code you're changing to be sure you aren't just taking the fast path
to making coverity shut up, reformat the changelogs as above, be ready
to tell us what fairly comprehensive testing you did on the result, and we
can take a pass at review on the list.


Thanks,
-Eric

Re: xfsdump cleaning patchset

[Jan Tulak]

On Mon, Oct 22, 2018 at 9:25 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>
> First, what testing was done on the patchset?

Except for xfstests, I tried some combinations of arguments by hand,
but not all possible ones.

>
> For Coverity issues, please use:
>
> Addresses-Coverity-ID: XYZ
>
> or even i.e.
>
> Addresses-Coverity-ID: XYZ ("Missing break in switch")

OK

[snip]

>
> I'd also kind of hoped to do more whitespace/style fixup before
> making code changes but that'll make your later patches impossible
> to apply, so maybe we'll just leave it for now.

Yeah, I realized that too, but it was already too late.

>
>
> I'm not a huge fan of "fix buffer overflows by making them arbitrarily
> bigger" - can we use i.e. snprintf to make sure it won't happen again,
> and maybe size them based on ... something, instead of "eh, [5000] is
> hopefully enough?"

I based the new size on gcc outputs, it said how much it can overflow.
But I checked the numbers only once or twice and didn't verify it for
other reports.

[snip]

>
>
> From spot-checking a few commits, please take another careful pass
> through to look at context and code history to be sure that you're
> really fixing the code properly and not just making coverity and/or
> gcc be quiet; for example in
>
> 0ab032a8 common/drive_minrmt.c: remove a dead code (CID 1056289)
>
> you saw that coverity spotted dead code and so you removed it, but it
> shouldn't be dead, it should be somewhere else, I think - look up about
> 30 lines from your change for a hint.  :)
>

I see...

>
> For some of the resource leak fixes, you might consider using something
> like "goto out_free;" rather than adding cleanup repeatedly at multiple
> return points.  Sometimes that's cleaner.

OK, I will consider that.


>
>
> TBH each of these is going to take careful review, nobody really
> knows how xfsdump works anymore so we'll have to be very careful that
> nothing breaks.
>
> Anyway, give it another check through, make sure you understand the
> code you're changing to be sure you aren't just taking the fast path
> to making coverity shut up, reformat the changelogs as above, be ready
> to tell us what fairly comprehensive testing you did on the result, and we
> can take a pass at review on the list.
>
>

Sure. Well, back to the drawing board. :-)


Thanks,
Jan


-- 
Jan Tulak

Re: xfsdump cleaning patchset

[Eric Sandeen]

On 10/23/18 3:16 AM, Jan Tulak wrote:
>> I'm not a huge fan of "fix buffer overflows by making them arbitrarily
>> bigger" - can we use i.e. snprintf to make sure it won't happen again,
>> and maybe size them based on ... something, instead of "eh, [5000] is
>> hopefully enough?"
> I based the new size on gcc outputs, it said how much it can overflow.
> But I checked the numbers only once or twice and didn't verify it for
> other reports.

It's always possible that I'm wrong and I'm missing something.  :)
What did gcc say for the cases I pointed out?

> Sure. Well, back to the drawing board. 

Eh not entirely, just recheck some stuff.  Just make sure you keep
the big picture in mine when you're looking at coverity reports.

Thanks,
-Eric

Re: xfsdump cleaning patchset

[Jan Tulak]

On Tue, Oct 23, 2018 at 2:10 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>
>
>
> On 10/23/18 3:16 AM, Jan Tulak wrote:
> >> I'm not a huge fan of "fix buffer overflows by making them arbitrarily
> >> bigger" - can we use i.e. snprintf to make sure it won't happen again,
> >> and maybe size them based on ... something, instead of "eh, [5000] is
> >> hopefully enough?"
> > I based the new size on gcc outputs, it said how much it can overflow.
> > But I checked the numbers only once or twice and didn't verify it for
> > other reports.
>
> It's always possible that I'm wrong and I'm missing something.  :)
> What did gcc say for the cases I pointed out?

For the txt[256] -> 300:

stobj.c: In function ‘stobjstrm_highlight’:
stobj.c:227:55: warning: ‘%s’ directive output may be truncated
writing up to 255 bytes into a regi
on of size between 230 and 231 [-Wformat-truncation=]
  snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
                                                       ^~
stobj.c:227:2: note: ‘snprintf’ output between 26 and 282 bytes into a
destination of size 256
  snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   (stobjstrm->st_interrupted == BOOL_TRUE) ? "yes" : "no",
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   stobjstrm->st_cmdarg);
   ~~~~~~~~~~~~~~~~~~~~~

So, I took the 282, added something, and rounded it to a number that
felt good and wasn't much larger than that.

Thanks,
Jan

-- 
Jan Tulak

Re: xfsdump cleaning patchset

[Eric Sandeen]

On 10/23/18 8:15 AM, Jan Tulak wrote:
> On Tue, Oct 23, 2018 at 2:10 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>>
>>
>>
>> On 10/23/18 3:16 AM, Jan Tulak wrote:
>>>> I'm not a huge fan of "fix buffer overflows by making them arbitrarily
>>>> bigger" - can we use i.e. snprintf to make sure it won't happen again,
>>>> and maybe size them based on ... something, instead of "eh, [5000] is
>>>> hopefully enough?"
>>> I based the new size on gcc outputs, it said how much it can overflow.
>>> But I checked the numbers only once or twice and didn't verify it for
>>> other reports.
>>
>> It's always possible that I'm wrong and I'm missing something.  :)
>> What did gcc say for the cases I pointed out?
> 
> For the txt[256] -> 300:
> 
> stobj.c: In function ‘stobjstrm_highlight’:
> stobj.c:227:55: warning: ‘%s’ directive output may be truncated
> writing up to 255 bytes into a region of size between 230 and 231 [-Wformat-truncation=]
>   snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
>                                                        ^~
> stobj.c:227:2: note: ‘snprintf’ output between 26 and 282 bytes into a
> destination of size 256
>   snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    (stobjstrm->st_interrupted == BOOL_TRUE) ? "yes" : "no",
>    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    stobjstrm->st_cmdarg);
>    ~~~~~~~~~~~~~~~~~~~~~
> 
> So, I took the 282, added something, and rounded it to a number that
> felt good and wasn't much larger than that.

Ok, I don't think that's one of the ones I commented on ;)

But let's look at this one.  For this string:

  "interrupted: %s, cmdarg: %s"

Treating the "%s" as 0 chars, we have 23 chars + NUL, so that's 24.

st_cmdarg is char st_cmdarg[INV_STRLEN] and INV_STRLEN is 128.

The "interrupted" string is at most 3 chars.

So we have 24+3+128=155 chars max AFAICT, maybe 156 w/ the NUL.

What are we missing?

Oh.  INV_STRLEN is defined differently in different places:

#define INV_STRLEN 128

and elsewhere

#define INV_STRLEN GLOBAL_HDR_STRING_SZ
#define GLOBAL_HDR_STRING_SZ 0x100

Ugh.  So, in this case, it would be much more foolproof to not pick
a magic number like "300" but instead do something like:

txt[INV_STRLEN+32]

which ensures we have enough for st_cmdarg[INV_STRLEN], as well as the more
obvious/easily-countable literal string lengths in the other partsof the message.
That way if somebody changes the INV_STRLEN for any reason, all the code still works.

(not sure +32 is sufficient, I didn't count closely, but you get the idea).

A little extra space is fine, I'm not saying we should count to the nearest
character! :)  But if a big part of the length depends an argument with a length
that depends on a defined macro, let's use that macro so the code doesn't break again later.

-eric

Re: xfsdump cleaning patchset

[Jan Tulak]

On Tue, Oct 23, 2018 at 3:43 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>
> On 10/23/18 8:15 AM, Jan Tulak wrote:
> > On Tue, Oct 23, 2018 at 2:10 PM Eric Sandeen <sandeen@sandeen.net> wrote:

> >> It's always possible that I'm wrong and I'm missing something.  :)
> >> What did gcc say for the cases I pointed out?
> >
> > For the txt[256] -> 300:
> >
> > stobj.c: In function ‘stobjstrm_highlight’:
> > stobj.c:227:55: warning: ‘%s’ directive output may be truncated
> > writing up to 255 bytes into a region of size between 230 and 231 [-Wformat-truncation=]
> >   snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
> >                                                        ^~
> > stobj.c:227:2: note: ‘snprintf’ output between 26 and 282 bytes into a
> > destination of size 256
> >   snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
> >   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    (stobjstrm->st_interrupted == BOOL_TRUE) ? "yes" : "no",
> >    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    stobjstrm->st_cmdarg);
> >    ~~~~~~~~~~~~~~~~~~~~~
> >
> > So, I took the 282, added something, and rounded it to a number that
> > felt good and wasn't much larger than that.
>
> Ok, I don't think that's one of the ones I commented on ;)

You mentioned multiple cases, so I picked the first one I found. :-)
If you ask for the 1024 -> 5000 bytes buffer, then it is probably this one:

invidx.c:253:37: warning: ‘%s’ directive output may be truncated
writing up to 4095 bytes into a r$
gion of size 1020 [-Wformat-truncation=]
   snprintf(cmd, sizeof(cmd), "cp %s %s", stobjfile, dst_stobjfile);
                                     ^~              ~~~~~~~~~~~~~
invidx.c:253:3: note: ‘snprintf’ output 5 or more bytes (assuming
4100) into a destination of size
1024
   snprintf(cmd, sizeof(cmd), "cp %s %s", stobjfile, dst_stobjfile);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I checked the MAXPATHLEN and we have:

./man/man8/xfsrestore.8:775:must be 1023 characters (MAXPATHLEN) or less.
And also /common/fs.h:27:#define FS_MAXPATHLEN_DEFAULT 1024


The exact declaration of MAXPATHLEN is missing in my grep (probably
some magic created during build time), so, I'm not sure how exactly it
gets the 4100, but in other places, we repeatedly do things like
calloc( 1, 2 * MAXPATHLEN ), and if the default and manpage values are
ok and the 2* propagates here, it makes it 4096+4=4100 without \0.

>
> But let's look at this one.  For this string:
>
>   "interrupted: %s, cmdarg: %s"
>
> Treating the "%s" as 0 chars, we have 23 chars + NUL, so that's 24.
>
> st_cmdarg is char st_cmdarg[INV_STRLEN] and INV_STRLEN is 128.
>
> The "interrupted" string is at most 3 chars.
>
> So we have 24+3+128=155 chars max AFAICT, maybe 156 w/ the NUL.
>
> What are we missing?
>
> Oh.  INV_STRLEN is defined differently in different places:
>
> #define INV_STRLEN 128
>
> and elsewhere
>
> #define INV_STRLEN GLOBAL_HDR_STRING_SZ
> #define GLOBAL_HDR_STRING_SZ 0x100
>
> Ugh.  So, in this case, it would be much more foolproof to not pick
> a magic number like "300" but instead do something like:
>
> txt[INV_STRLEN+32]
>
> which ensures we have enough for st_cmdarg[INV_STRLEN], as well as the more
> obvious/easily-countable literal string lengths in the other partsof the message.
> That way if somebody changes the INV_STRLEN for any reason, all the code still works.
>
> (not sure +32 is sufficient, I didn't count closely, but you get the idea).

Yeah, that looks better.

Thanks,
Jan

>
> A little extra space is fine, I'm not saying we should count to the nearest
> character! :)  But if a big part of the length depends an argument with a length
> that depends on a defined macro, let's use that macro so the code doesn't break again later.
>
> -eric



--
Jan Tulak

Re: xfsdump cleaning patchset

[Eric Sandeen]

On 10/23/18 9:06 AM, Jan Tulak wrote:
> On Tue, Oct 23, 2018 at 3:43 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>>
>> On 10/23/18 8:15 AM, Jan Tulak wrote:
>>> On Tue, Oct 23, 2018 at 2:10 PM Eric Sandeen <sandeen@sandeen.net> wrote:
> 
>>>> It's always possible that I'm wrong and I'm missing something.  :)
>>>> What did gcc say for the cases I pointed out?
>>>
>>> For the txt[256] -> 300:
>>>
>>> stobj.c: In function ‘stobjstrm_highlight’:
>>> stobj.c:227:55: warning: ‘%s’ directive output may be truncated
>>> writing up to 255 bytes into a region of size between 230 and 231 [-Wformat-truncation=]
>>>   snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
>>>                                                        ^~
>>> stobj.c:227:2: note: ‘snprintf’ output between 26 and 282 bytes into a
>>> destination of size 256
>>>   snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s",
>>>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>    (stobjstrm->st_interrupted == BOOL_TRUE) ? "yes" : "no",
>>>    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>    stobjstrm->st_cmdarg);
>>>    ~~~~~~~~~~~~~~~~~~~~~
>>>
>>> So, I took the 282, added something, and rounded it to a number that
>>> felt good and wasn't much larger than that.
>>
>> Ok, I don't think that's one of the ones I commented on ;)
> 
> You mentioned multiple cases, so I picked the first one I found. :-)
> If you ask for the 1024 -> 5000 bytes buffer, then it is probably this one:
> 
> invidx.c:253:37: warning: ‘%s’ directive output may be truncated
> writing up to 4095 bytes into a r$
> gion of size 1020 [-Wformat-truncation=]
>    snprintf(cmd, sizeof(cmd), "cp %s %s", stobjfile, dst_stobjfile);
>                                      ^~              ~~~~~~~~~~~~~
> invidx.c:253:3: note: ‘snprintf’ output 5 or more bytes (assuming
> 4100) into a destination of size
> 1024
>    snprintf(cmd, sizeof(cmd), "cp %s %s", stobjfile, dst_stobjfile);
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I checked the MAXPATHLEN and we have:
> 
> ./man/man8/xfsrestore.8:775:must be 1023 characters (MAXPATHLEN) or less.
> And also /common/fs.h:27:#define FS_MAXPATHLEN_DEFAULT 1024

Except checking at build time I get:

invidx.c: In function ‘invidx_commit’:
invidx.c:125: note: #pragma message: MAXPATHLEN=4096

> 
> The exact declaration of MAXPATHLEN is missing in my grep (probably
> some magic created during build time), so, I'm not sure how exactly it
> gets the 4100, but in other places, we repeatedly do things like
> calloc( 1, 2 * MAXPATHLEN ), and if the default and manpage values are
> ok and the 2* propagates here, it makes it 4096+4=4100 without \0.

Right, so again, using the macro here makes it more foolproof.

Thanks,
-Eric