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

Comment out bogus code in XMPUtils.cpp #1902

Merged
merged 4 commits into from
Sep 19, 2021

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes #1901.

This loop is a very weird way to make sure that tmLocal.tm_year isn't less than 70. According to the comment, it's a defense against bad implementations of mktime. I am unconvinced that mktime wouldn't be able to handle dates before 1970, so I think the simplest solution is to comment this code out.

@kevinbackhouse kevinbackhouse added bug OSS-Fuzz Bug reported by https://google.github.io/oss-fuzz/ labels Sep 12, 2021
@kevinbackhouse kevinbackhouse added this to the v1.00 milestone Sep 12, 2021
@codecov
Copy link

codecov bot commented Sep 12, 2021

Codecov Report

Merging #1902 (8e0682f) into main (1b53899) will increase coverage by 0.02%.
The diff coverage is 73.80%.

❗ Current head 8e0682f differs from pull request most recent head bb9ff53. Consider uploading reports for the commit bb9ff53 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1902      +/-   ##
==========================================
+ Coverage   60.87%   60.89%   +0.02%     
==========================================
  Files          96       96              
  Lines       19041    19041              
  Branches     9726     9726              
==========================================
+ Hits        11591    11595       +4     
+ Misses       5138     5133       -5     
- Partials     2312     2313       +1     
Impacted Files Coverage Δ
src/convert.cpp 53.84% <73.80%> (+0.34%) ⬆️
src/value.cpp 73.03% <0.00%> (+0.30%) ⬆️

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 1b53899...bb9ff53. Read the comment docs.

@postscript-dev
Copy link
Collaborator

I am unconvinced that mktime wouldn't be able to handle dates before 1970, so I think the simplest solution is to comment this code out.

Is this of any use?
https://developercommunity.visualstudio.com/t/mktime-does-not-support-dates-before-jan-1-970/1208504

@kevinbackhouse
Copy link
Collaborator Author

@postscript-dev: Thanks for the link. The Microsoft people in that thread seem confident that their implementation works as intended, so it's unlikely that a date before 1970 is going to cause anything bad to happen. You might get a weird date, but I don't think it's going to cause a crash.

@kevinbackhouse
Copy link
Collaborator Author

I have updated this pull request to also check for integer overflow. The assignment tmLocal.tm_year = xmpTime->year - 1900 could overflow. In fact, that's what caused the fuzzer failure, not the long-running loop.

@postscript-dev
Copy link
Collaborator

Sorry for the delay to this, I will look at it tomorrow.

Copy link
Collaborator

@postscript-dev postscript-dev left a comment

Choose a reason for hiding this comment

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

LGTM.

As I am not as experienced as many Exiv2 contributors, it is better to ask me to review simpler changes. I will try and help where I can.

@kevinbackhouse
Copy link
Collaborator Author

@Mergifyio backport 0.27-maintenance

@mergify
Copy link
Contributor

mergify bot commented Sep 24, 2021

Command backport 0.27-maintenance: success

Backports have been created

@clanmills clanmills mentioned this pull request Sep 29, 2021
kevinbackhouse added a commit that referenced this pull request Sep 29, 2021
Comment out bogus code in XMPUtils.cpp (backport #1902)
@clanmills clanmills mentioned this pull request Sep 30, 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.

OSS-Fuzz: integer-overflow in XMPUtils::SetTimeZone
2 participants