GNU bug report logs - #23825
maint: avoid md5sum.c warning from bleeding-edge gcc's -Wstrict-overflow

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23825 in the body.
You can then email your comments to 23825 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: bug-coreutils <at> gnu.org
Subject: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Wed, 22 Jun 2016 07:37:07 -0700

[Message part 1 (text/plain, inline)]

Building with a recent gcc-7 failed, so I wrote the attached patch. I
think we'll never have 2^31 command line arguments, so "unsigned int"
will ll be ok:

    maint: avoid md5sum.c warning from bleeding-edge gcc's -Wstrict-overflow

    Avoid this warning/error from gcc:
      src/md5sum.c:870:3: error: assuming signed overflow does not occur \
      when simplifying conditional to constant [-Werror=strict-overflow]
    * src/md5sum.c (main): Use an unsigned variable as the loop index,
    rather than optind.

[0001-maint-avoid-md5sum.c-warning-from-bleeding-edge-gcc-.patch (text/x-patch, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#23825: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Thu, 23 Jun 2016 09:13:53 +0200

[Message part 1 (text/plain, inline)]

On 06/22/2016 04:37 PM, Jim Meyering wrote:
> Building with a recent gcc-7 failed, so I wrote the attached patch. I
> think we'll never have 2^31 command line arguments
I would rather fix the code so that it works even if argc is initially 
INT_MAX; the code currently has undefined behavior in that case. I have 
drafted a patch for this (attached; does not have changelog entries yet) 
and will test it a bit. I assume this will fix the warning (don't have 
gcc-7 offhand so can't test this).

As the patch shows, a couple of other coreutils programs have a similar 
bug; I'm a bit surprised gcc-7 didn't complain about them.

Incidentally, 'yes' has a different bug: it mishandles the case where 
'write' succeeds but returns a value less than the buffer size. I'll try 
to look into that too. Simplest would be to use stdio (the comments 
indicate this has performance issues but I don't know what they are, 
anyway correctness trumps performance).

[coreutils.diff (text/x-patch, attachment)]

Message #11 received at 23825 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, 23825 <at> debbugs.gnu.org
Subject: Re: bug#23825: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Thu, 23 Jun 2016 14:26:41 +0100

[Message part 1 (text/plain, inline)]

On 23/06/16 08:13, Paul Eggert wrote:
> Incidentally, 'yes' has a different bug: it mishandles the case where 
> 'write' succeeds but returns a value less than the buffer size. I'll try 
> to look into that too. Simplest would be to use stdio (the comments 
> indicate this has performance issues but I don't know what they are, 
> anyway correctness trumps performance).

Good spot on the yes(1) write(2) bug.
Shouldn't impact too often due to smallish BUFSIZ,
and subsequent writes catching ENOSPC,
but definitely could cause issues.

The attached should fix that up, and keep the same
performance characteristics.

cheers,
Pádraig

[yes-short-write.patch (text/x-patch, attachment)]

Message #14 received at 23825 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, 23825 <at> debbugs.gnu.org
Subject: Re: bug#23825: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Thu, 23 Jun 2016 11:29:06 -0600

On Thu, Jun 23, 2016 at 7:26 AM, Pádraig Brady <P <at> draigbrady.com> wrote:
> On 23/06/16 08:13, Paul Eggert wrote:
>> Incidentally, 'yes' has a different bug: it mishandles the case where
>> 'write' succeeds but returns a value less than the buffer size. I'll try
>> to look into that too. Simplest would be to use stdio (the comments
>> indicate this has performance issues but I don't know what they are,
>> anyway correctness trumps performance).
>
> Good spot on the yes(1) write(2) bug.
> Shouldn't impact too often due to smallish BUFSIZ,
> and subsequent writes catching ENOSPC,
> but definitely could cause issues.
>
> The attached should fix that up, and keep the same
> performance characteristics.

Hi Pádraig,
Thank you. That looks fine, modulo a formatting nit. I'd correct it like this:

 -const char* pwrite = buf
 +const char *pwrite = buf

Hi Paul,
Thank you, too.
I confirmed that all compile (using yesterday's gcc-7 snapshot) with
your patch. My only reservation is barely worth mentioning: the new
variable names, operandp and operand_lim are a bit too long for my
taste (at least in the files where all uses are very near the
definitions). On the other hand, they are also more readable than
alternatives that came to mind, and in some cases are used far enough
away from point of definition that one can justify that added length.

Message #17 received at 23825 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>, Pádraig Brady
 <P <at> draigbrady.com>
Cc: 23825 <at> debbugs.gnu.org
Subject: Re: bug#23825: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Thu, 23 Jun 2016 20:19:56 +0200

On 06/23/2016 07:29 PM, Jim Meyering wrote:
> My only reservation is barely worth mentioning: the new
> variable names, operandp and operand_lim are a bit too long for my
> taste (at least in the files where all uses are very near the
> definitions). On the other hand, they are also more readable than
> alternatives that came to mind, and in some cases are used far enough
> away from point of definition that one can justify that added length.

Exactly the thoughts that ran through my head!  I couldn't think of 
better names either.  The word "operand" came from the technical term 
used in the POSIX spec for utility arguments.  If someone else can come 
up with a better (shorter) name I'm all for using it.  In the meantime I 
installed the patch.

Message #20 received at 23825 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>, Pádraig Brady
 <P <at> draigbrady.com>
Cc: 23825 <at> debbugs.gnu.org
Subject: Re: bug#23825: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Sat, 25 Jun 2016 01:08:58 +0200

I used full_write to simplify that, and also changed it to use just 
full_write and not a mixture of full_write and stdio.

Message #23 received at 23825 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Jim Meyering <jim <at> meyering.net>
Cc: 23825 <at> debbugs.gnu.org
Subject: Re: bug#23825: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Sat, 25 Jun 2016 04:26:16 +0100

On 25/06/16 00:08, Paul Eggert wrote:
> I used full_write to simplify that, and also changed it to use just 
> full_write and not a mixture of full_write and stdio.

and clever reuse of argv[] for large operands.

thanks!
Pádraig

Message #26 received at 23825 <at> debbugs.gnu.org (full text, mbox):

From: Assaf Gordon <assafgordon <at> gmail.com>
To: 23825 <at> debbugs.gnu.org
Subject: Re: bug#23825: maint: avoid md5sum.c warning from bleeding-edge gcc's
 -Wstrict-overflow
Date: Sun, 28 Oct 2018 00:13:11 -0600

tags 23825 fixed
close 23825
stop

(triaging old bugs)

The two discussed issues pushed here:

maint: work even if argc == INT_MAX
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=818b2f48974298065a43a8a2d5355e4aaa65c09d

yes: handle short writes
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=5845664c8c61faf004eb3ef9979e770f794108c1


Closing as "fixed".

-assaf

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.