Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix integer overflow in PanasonicMakerNote::printAccelerometer #2007

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes: #2006

The subtraction in this code is getting flagged by OSS-Fuzz because it can overflow. It's obviously harmless because it only affects the value that gets printed, so this fix is just to make OSS-Fuzz happy.

This subtraction code is very strange though. According to the comment, it's a conversion from unsigned to signed. The comment doesn't specifically say, but it looks like the code is expecting the value to be a uint16_t. But it's weird because it's subtracting 0xffff, rather than 0x10000 which is what I would expect for two's complement arithmetic.

I have replaced the weird arithmetic with a simple static_cast, because that's what makes sense to me. Unfortunately, we do not have any test files that trigger this code.

@kevinbackhouse kevinbackhouse added bug OSS-Fuzz Bug reported by https://google.github.io/oss-fuzz/ labels Nov 28, 2021
@codecov
Copy link

codecov bot commented Nov 28, 2021

Codecov Report

Merging #2007 (35f48ae) into main (fde8ed0) will increase coverage by 0.02%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2007      +/-   ##
==========================================
+ Coverage   61.16%   61.18%   +0.02%     
==========================================
  Files          96       96              
  Lines       19255    19252       -3     
  Branches     9862     9862              
==========================================
+ Hits        11777    11780       +3     
+ Misses       5135     5129       -6     
  Partials     2343     2343              
Impacted Files Coverage Δ
src/panasonicmn_int.cpp 43.13% <33.33%> (+4.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fde8ed0...35f48ae. Read the comment docs.

@clanmills
Copy link
Collaborator

Pass me the barf bag:

i = i - ((i & 0x8000) >> 15) * 0xffff;

I'm not clever enough to know what that's meant to do. I see it's repeated:

641 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance $ git blame src/panasonicmn_int.cpp | grep 0x8000
717d789c8 src/panasonicmn.cpp     (Andreas Huggel   2010-08-30 19:33:07 +0000 520)         TagInfo(0x8000, "MakerNoteVersion", N_("MakerNote Version"), N_("MakerNote version"), panasonicId, makerTags, undefined, -1, printExifVersion),
c41988539 src/panasonicmn.cpp     (nkbj             2014-09-08 03:57:56 +0000 697)         i = i - ((i & 0x8000) >> 15) * 0xffff;
c41988539 src/panasonicmn.cpp     (nkbj             2014-09-08 03:57:56 +0000 705)         i = i - ((i & 0x8000) >> 15) * 0xffff;
c41988539 src/panasonicmn.cpp     (nkbj             2014-09-08 03:57:56 +0000 719)         i = i - ((i & 0x8000) >> 15) * 0xffff;
642 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance $ 

Copy link
Collaborator

@kmilos kmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinbackhouse kevinbackhouse merged commit 65fad9d into Exiv2:main Dec 6, 2021
@kevinbackhouse kevinbackhouse added this to the v1.00 milestone Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug OSS-Fuzz Bug reported by https://google.github.io/oss-fuzz/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UBSAN error due to integer overflow in PanasonicMakerNote::printAccelerometer
3 participants