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

Avoid negative integer overflow when chunkLength == 0 #794

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes #789.

@codecov
Copy link

codecov bot commented Apr 25, 2019

Codecov Report

Merging #794 into 0.27-maintenance will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           0.27-maintenance     #794      +/-   ##
====================================================
+ Coverage             62.67%   62.68%   +<.01%     
====================================================
  Files                   156      156              
  Lines                 21543    21545       +2     
====================================================
+ Hits                  13503    13505       +2     
  Misses                 8040     8040
Impacted Files Coverage Δ
src/pngimage.cpp 80.86% <100%> (+0.11%) ⬆️

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 f05d100...206c357. Read the comment docs.

src/pngimage.cpp Outdated Show resolved Hide resolved
@piponazo piponazo self-requested a review April 25, 2019 16:28
piponazo
piponazo previously approved these changes Apr 25, 2019
Copy link
Member

@D4N D4N left a comment

Choose a reason for hiding this comment

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

Please squash these two commits into one: 1e0dec2 and 9192d62.

I'd really prefer a solution that does not incur another exception to be thrown. If you can find a way: that'd be great, but I'm fine with the PR if you prefer not to touch this further.

@kevinbackhouse
Copy link
Collaborator Author

@D4N: an alternative solution is disallow chunkLength == 0 on line 433. But I didn't do that because I wasn't sure if chunkLength == 0 is invalid when chunkType is something different, like "IEND".

Why do you not want to throw an exception? Is it a performance thing?

@kevinbackhouse kevinbackhouse force-pushed the bugfix_789_PngImage_readMetadata branch 2 times, most recently from 43d1868 to f6cca78 Compare April 25, 2019 20:43
@mergify mergify bot dismissed piponazo’s stale review April 30, 2019 08:35

Pull request has been modified.

@clanmills clanmills added this to the v0.27.2 milestone Apr 30, 2019
@kevinbackhouse kevinbackhouse force-pushed the bugfix_789_PngImage_readMetadata branch from c9078ac to 3259224 Compare May 2, 2019 09:17
piponazo
piponazo previously approved these changes May 4, 2019
@piponazo
Copy link
Collaborator

piponazo commented May 4, 2019

Thanks for the contribution! LGTM.

Would you mind to rebase the branch in top 0.27-maintenance ?

@D4N D4N self-requested a review May 5, 2019 14:41
D4N
D4N previously approved these changes May 5, 2019
Copy link
Member

@D4N D4N left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution!

@D4N D4N force-pushed the bugfix_789_PngImage_readMetadata branch from 3259224 to 6f49df2 Compare May 5, 2019 14:44
@mergify mergify bot dismissed stale reviews from piponazo and D4N May 5, 2019 14:45

Pull request has been modified.

@D4N
Copy link
Member

D4N commented May 5, 2019

@kevinbackhouse I've rebased the changes in your branch on top of 0.27-maintenance.

@D4N D4N requested review from piponazo and D4N May 5, 2019 14:45
D4N
D4N previously approved these changes May 5, 2019
@D4N D4N force-pushed the bugfix_789_PngImage_readMetadata branch from 6f49df2 to 206c357 Compare May 5, 2019 18:12
@mergify mergify bot dismissed D4N’s stale review May 5, 2019 18:12

Pull request has been modified.

@D4N
Copy link
Member

D4N commented May 5, 2019

@piponazo @kevinbackhouse any objections to merging this?

@piponazo piponazo merged commit 8cd95e2 into Exiv2:0.27-maintenance May 6, 2019
@kevinbackhouse kevinbackhouse deleted the bugfix_789_PngImage_readMetadata branch September 18, 2021 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants