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 <aa...@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@lists.freedesktop.org > >>>>> https://lists.freedesktop.org/mailman/listinfo/poppler _______________________________________________ poppler mailing list poppler@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/poppler