Skip to content

Commit

Permalink
Avoid shifts of negative values inflateMark().
Browse files Browse the repository at this point in the history
The C standard says that bit shifts of negative integers is
undefined.  This casts to unsigned values to assure a known
result.
  • Loading branch information
madler committed Sep 6, 2015
1 parent 27ef026 commit e54e129
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions inflate.c
Original file line number Diff line number Diff line change
Expand Up @@ -1506,9 +1506,10 @@ z_streamp strm;
{
struct inflate_state FAR *state;

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

This comment has been minimized.

Copy link
@Dmitry-Me

Dmitry-Me Oct 1, 2015

What's the need for (unsigned long)0 - 1? Why not simply (unsigned long)-1?

This comment has been minimized.

Copy link
@madler

madler Oct 6, 2015

Author Owner

To avoid any uncertainty on the part of the reader. There is no question about what (unsigned long)0 is, and there is no question about what happens when you subtract one from it. Since -1 is not unsigned, figuring out what (unsigned long)-1 is expected to become might requiring referring to the type conversion rules of C.

This comment has been minimized.

Copy link
@Dmitry-Me

Dmitry-Me Oct 6, 2015

Okay, why not just 0UL?

This comment has been minimized.

Copy link
@madler

madler Oct 6, 2015

Author Owner

That would work as well. I like to spell it out.

This comment has been minimized.

Copy link
@jibsen

jibsen Aug 22, 2016

I guess technically, since the unsigned long value is outside the range of long, the result of the cast back to long is implementation defined (C11 6.3.1.3p3).

What about -(1L << 16), as suggested here?

This comment has been minimized.

Copy link
@jpark37

jpark37 Oct 11, 2016

Ran into this recently as well. I would prefer the solution referenced by jibsen to avoid violating the standard.

state = (struct inflate_state FAR *)strm->state;
return ((long)(state->back) << 16) +
return (long)(((unsigned long)((long)state->back)) << 16) +
(state->mode == COPY ? state->length :
(state->mode == MATCH ? state->was - state->length : 0));
}
Expand Down

2 comments on commit e54e129

@madler
Copy link
Owner Author

@madler madler commented on e54e129 Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. See 2edb94a

@chandanbn
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is CVE-2016-9842.

Please sign in to comment.