[poppler] poppler/Stream.cc

[Albert Astals Cid]

El dissabte, 26 de maig de 2018, a les 11:17:31 CEST, Adam Reichold va 
escriure:
> Hello,
> 
> Am 26.05.2018 um 10:59 schrieb Albert Astals Cid:
> > El dimecres, 23 de maig de 2018, a les 21:46:02 CEST, Adam Reichold va 
escriure:
> >> Hello again,
> >> 
> >> attached the patch. It declares inputBuf as unsigned so all bit shifts
> >> happen on unsigned values. ctest at least seems to be happy.
> > 
> > Interestingly the automagic fuzzer service says that the issue with
> > LZWStream::getCode doing left shift on negative values has been fixed by
> > a commit in this range
> > https://cgit.freedesktop.org/poppler/poppler/diff/?id2=76820f5ab932a9ed18
> > 913bc7d1a452ddf060c133&id=f966b9096d046aaee4891de11f74207218cc929b
> Are you sure this is exhaustive? 

It's totally not exhaustive :D

> I suspect the fuzzer randomly explores
> the input space, probably coverage guided? So couldn't it be that it
> just did not hit the issue again yet, since from looking at the code,
> inputBuf could very well become negative when considered as a signed
> integer depending on the specific input bytes.

You're right, actually i have "real" pdf files that used to depend on the gcc 
implementation of the undefined behaviour (that's how i realized that my 
initial change was causing a regression), so I've commited your change now :)

Cheers,
  Albert

> 
> Best regards,
> Adam
> 
> > So i guess for not better not to touch this code.
> > 
> > Thanks for the patch though :)
> > 
> > Cheers,
> > 
> >   Albert
> >> 
> >> It does build without the casts as well but I am not completely sure
> >> about the language legalese behind this and hence left them in and also
> >> for explicitness.
> >> 
> >> Proper fix would probably be to converted all of the LZW decoding to use
> >> unsigned values.
> >> 
> >> Best regards,
> >> Adam
> >> 
> >> Am 23.05.2018 um 21:24 schrieb Albert Astals Cid:
> >>> El dimecres, 23 de maig de 2018, a les 8:57:27 CEST, Adam Reichold va
> >>> 
> >>> escriure:
> >>>> Hello,
> >>>> 
> >>>> maybe the simplest solution would to turn inputBuf into an unsigned int
> >>>> and convert to signed int after extracting the bits out of it?
> >>> 
> >>> Yeah that sounds like a plan, could you try to produce a patch so i can
> >>> run it through regtest?
> >>> 
> >>> Cheers,
> >>> 
> >>>   Albert
> >>>> 
> >>>> Best regards,
> >>>> Adam
> >>>> 
> >>>> Am 23.05.2018 um 00:24 schrieb Albert Astals Cid:
> >>>>>  poppler/Stream.cc |    4 +---
> >>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>>>> 
> >>>>> New commits:
> >>>>> commit 58e056c4b15f262b7715f8061d6885eb80044d0d
> >>>>> Author: Albert Astals Cid <aacid at kde.org>
> >>>>> Date:   Wed May 23 00:23:19 2018 +0200
> >>>>> 
> >>>>>     Revert 31c3832b996acbf04ea833e304d7d21ac4533a57
> >>>>>     
> >>>>>     So shifting left negative values is undefined behaviour according
> >>>>>     to
> >>>>>     the
> >>>>>     spec but if we don't do it we break, so we seem to be depending on
> >>>>>     this
> >>>>>     undefined behaviour, will try to figure out a better fix
> >>>>> 
> >>>>> diff --git a/poppler/Stream.cc b/poppler/Stream.cc
> >>>>> index b6bfd838..4f075c12 100644
> >>>>> --- a/poppler/Stream.cc
> >>>>> +++ b/poppler/Stream.cc
> >>>>> @@ -1445,9 +1445,7 @@ int LZWStream::getCode() {
> >>>>> 
> >>>>>    while (inputBits < nextBits) {
> >>>>>    
> >>>>>      if ((c = str->getChar()) == EOF)
> >>>>>      
> >>>>>        return EOF;
> >>>>> 
> >>>>> -    if (likely(inputBuf >= 0)) {
> >>>>> -        inputBuf = (inputBuf << 8) | (c & 0xff);
> >>>>> -    }
> >>>>> +    inputBuf = (inputBuf << 8) | (c & 0xff);
> >>>>> 
> >>>>>      inputBits += 8;
> >>>>>    
> >>>>>    }
> >>>>>    code = (inputBuf >> (inputBits - nextBits)) & ((1 << nextBits) -
> >>>>>    1);
> >>>>> 
> >>>>> _______________________________________________
> >>>>> poppler mailing list
> >>>>> poppler at lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/poppler