Bug 71704

Summary: Some improvements in reading the hints table
Product: poppler Reporter: Hib Eris <hib>
Component: generalAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED MOVED QA Contact:
Severity: normal    
Priority: medium CC: hib
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: 0001-Remove-unused-assignments-in-Hints.cc.patch
0002-Validate-input-from-page-offset-hints-table.patch
0003-Read-at-most-32-bits-with-Hints-readBits.patch

Description Hib Eris 2013-11-17 16:34:32 UTC
Created attachment 89366 [details]
0001-Remove-unused-assignments-in-Hints.cc.patch

When trying out the clang static analyzer on poppler's code, I stubbled upon a few minor issues in the code for parsing the hints table.

Please consider the following patches.
Comment 1 Hib Eris 2013-11-17 16:35:05 UTC
Created attachment 89368 [details] [review]
0002-Validate-input-from-page-offset-hints-table.patch
Comment 2 Hib Eris 2013-11-17 16:35:43 UTC
Created attachment 89369 [details] [review]
0003-Read-at-most-32-bits-with-Hints-readBits.patch
Comment 3 Albert Astals Cid 2013-11-18 22:54:31 UTC
I am not sure i understand patch 3, can you explain a bit more?
Comment 4 Hib Eris 2013-11-19 08:30:55 UTC
The function readBits returns an Guint. When this Guint is a data type that can only contain 32 bits, the returned value cannot represent all the bits read when reading more than 32 bits. Thus we should use readBits to read a maximum of 32 bits at a time. To read 128 bits, we can read 4 x 32 = 128 bits.
Comment 5 Albert Astals Cid 2013-11-19 23:52:28 UTC
Right, but as far as i can see you're discarding those bits (since you're not assigning the readBits call to anything) so what's the problem?
Comment 6 Hib Eris 2013-11-20 09:38:54 UTC
(In reply to comment #5)
> Right, but as far as i can see you're discarding those bits (since you're
> not assigning the readBits call to anything) so what's the problem?

That is correct, there is not really a problem with it right now because the shared object signature is not used anywhere.

Using readBits(128) to _skip _over the shared object signature works, but when you would use it to _read_ it, it would not work / give you undefined behavior.

As the readBits function will give undefined results when called with n > 32 I think it would be prudent to not use it with n = 128.

It may be even better to have readBits assert (n <= 32).
Comment 7 Albert Astals Cid 2013-11-20 21:01:21 UTC
(In reply to comment #6)
> Using readBits(128) to _skip _over the shared object signature works, but
> when you would use it to _read_ it, it would not work / give you undefined
> behavior.

Exactly and since what we're doing is skipping and not reading (and it's even kind of commented in the file) i think it's better to have a single readBits than 4 of them, i mean, if I look at the new code, i will wonder why there's 4 calls that do "nothing" when there could be only one that does the same amount of "nothing".
Comment 8 GitLab Migration User 2018-08-21 10:43:04 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/343.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.