Fix UBSAN failure caused by left-shift of negative number #1921
Conversation
…l?id=39060 Fix UBSAN failure caused by left shift of a negative number.
Codecov Report
@@ Coverage Diff @@
## main #1921 +/- ##
==========================================
- Coverage 61.13% 61.12% -0.01%
==========================================
Files 96 96
Lines 19051 19068 +17
Branches 9729 9745 +16
==========================================
+ Hits 11647 11656 +9
- Misses 5090 5093 +3
- Partials 2314 2319 +5
Continue to review full report at Codecov.
|
23314b3 to
fc07f18
Compare
|
In the third commit, I added a CodeQL query which warns about other signed shifts in the codebase. It has generated quite a few warnings, which I plan to fix in a separate PR, so that the warnings show up as "closed" on the Security tab. |
kmilos
left a comment
There was a problem hiding this comment.
What about all the other warnings now?
src/pentaxmn_int.cpp
Outdated
| { | ||
| /* I choose same format as is used inside EXIF itself */ | ||
| os << ((value.toLong(0) << 8) + value.toLong(1)); | ||
| os << ((static_cast<uint64_t>(value.toLong(0)) << 8) + value.toLong(1)); |
There was a problem hiding this comment.
uint32_t doesn't cut it here?
There was a problem hiding this comment.
Actually uint16_t is probably sufficient for the intended purpose here. I think this is probably a similar scenario to #1706. The value is supposed to be a byte, but there's no validation on that, so a malicious file can contain a long instead. For the valid cases, we only need to extract the byte.
I'll change it.
|
I can put the CodeQL query in a separate PR if you prefer. It has generated a lot of spam in the diff. |
Fixes: #1920
Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39060
Add a cast to
uint64_tto fix a UBSAN failure.