Page MenuHomeFreeBSD

Fix undefined shift of a negative value in libz
ClosedPublic

Authored by dim on Aug 8 2015, 10:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 5:33 PM
Unknown Object (File)
Mon, Nov 25, 12:30 AM
Unknown Object (File)
Mon, Nov 25, 12:30 AM
Unknown Object (File)
Sat, Nov 23, 7:38 AM
Unknown Object (File)
Nov 2 2024, 1:54 AM
Unknown Object (File)
Oct 15 2024, 12:09 AM
Unknown Object (File)
Oct 4 2024, 6:57 PM
Unknown Object (File)
Oct 4 2024, 8:39 AM
Subscribers

Details

Summary

Newer snapshots of clang trunk produce the following warning on libz's
inflateMark() function:

lib/libz/inflate.c:1507:61: error: shifting a negative signed value is
undefined [-Werror,-Wshift-negative-value]
    if (strm == Z_NULL || strm->state == Z_NULL) return -1L << 16;
                                                        ~~~ ^
1 error generated.

Indeed the standard says that this is undefined, but asking around, it
seems that almost everybody will interpret this as a two's complement
shift, e.g. equivalent to:

(unsigned long)-1L << 16

or shorter:

~0UL << 16

Resulting in the actual value -65536. I would like to change it to this
last notation.

I'm not sure if it is needed (or wise) to also update the documentation
comment for inflateMark(), though. Any comments about that?

Test Plan

There is no functional change, except squelching -Werror.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dim retitled this revision from to Fix undefined shift of a negative value in libz.
dim updated this object.
dim edited the test plan for this revision. (Show Details)
dim added reviewers: delphij, bapt.

Alternatively, we could write:

if (strm == Z_NULL || strm->state == Z_NULL) return -(1L << 16);

Which may look a little nicer, and is equivalent.

Use -(1L << 16) instead, which seems more natural.

This revision is now accepted and ready to land.Aug 13 2015, 7:53 PM

I like the change, but would like to ask upstream for their opinion so this would be included in the next update, can you hold it for a little bit while I'm asking them?

I like the change, but would like to ask upstream for their opinion so this would be included in the next update, can you hold it for a little bit while I'm asking them?

Sure, but I think they'll not put out a new release for this. :)

In D3344#68817, @dim wrote:

I like the change, but would like to ask upstream for their opinion so this would be included in the next update, can you hold it for a little bit while I'm asking them?

Sure, but I think they'll not put out a new release for this. :)

Well, if we know it's fixed in upstream (or at least will be fixed in upstream), we can commit the change to vendor branch and do a merge. This way, future merges would be easier.

If there is no responses by next Wednesday, I think we should execute plan B and proceed with the commit to head/ only. I'll take care for future merge conflicts, if there would be any.

delphij edited edge metadata.
This revision was automatically updated to reflect the committed changes.