[poppler] poppler/Stream.cc

[Adam Reichold]

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=76820f5ab932a9ed18913bc7d1a452ddf060c133&id=f966b9096d046aaee4891de11f74207218cc929b

Are you sure this is exhaustive? 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.

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
> 
> 
> 
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/poppler/attachments/20180526/4355ca8a/attachment.sig>