| Summary: | Some improvements in reading the hints table | ||
|---|---|---|---|
| Product: | poppler | Reporter: | Hib Eris <hib> |
| Component: | general | Assignee: | 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 |
||
Created attachment 89368 [details] [review] 0002-Validate-input-from-page-offset-hints-table.patch Created attachment 89369 [details] [review] 0003-Read-at-most-32-bits-with-Hints-readBits.patch I am not sure i understand patch 3, can you explain a bit more? 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. 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? (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). (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". -- 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.
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.