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

Use vector::at() rather than operator[] #1735

Merged
merged 4 commits into from
Jun 26, 2021

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes #1706

The problem in #1706 is that PentaxMakerNote::printDate() expects a value of type undefined, but it is given a value of type unsignedLong instead. That leads to an out-of-bounds access of a std::vector, which triggers an assertion failure.

It looks difficult to prevent printDate, and similar functions like printTime, from getting called on values with the wrong type. So I think the most reliable way of preventing a crash is by using vector::at() rather than operator[]. The difference is that vector::at() throws an exception if the index is out-of-bounds. It is easy to catch the exception and handle the error gracefully.

@kevinbackhouse kevinbackhouse added the forward-to-main Forward changes in a 0.28.x PR to main with Mergify label Jun 21, 2021
@kevinbackhouse kevinbackhouse linked an issue Jun 21, 2021 that may be closed by this pull request
src/exif.cpp Show resolved Hide resolved
Copy link
Member

@hassec hassec left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@hassec hassec merged commit f4d3adb into Exiv2:0.27-maintenance Jun 26, 2021
mergify bot pushed a commit that referenced this pull request Jun 26, 2021
* Regression test for #1706

* Use vector::at() rather than operator[].

* Print to stderr when exception is caught and EXIV2_DEBUG_MESSAGES is enabled.

* Check that it prints "Bad value" for the date.

(cherry picked from commit f4d3adb)

# Conflicts:
#	src/value.cpp
@hassec hassec added this to the v0.27.5 milestone Jun 26, 2021
hassec added a commit that referenced this pull request Jun 27, 2021
* fix: use vector::at() rather than operator[] (#1735)

* Regression test for #1706

* Use vector::at() rather than operator[].

* Print to stderr when exception is caught and EXIV2_DEBUG_MESSAGES is enabled.

* Check that it prints "Bad value" for the date.

(cherry picked from commit f4d3adb)

# Conflicts:
#	src/value.cpp

* fix merge conflicts from mergify backport

Co-authored-by: Kevin Backhouse <kevinbackhouse@github.com>
Co-authored-by: Christoph Hasse <hassec@users.noreply.github.com>
@kevinbackhouse
Copy link
Collaborator Author

This is the CodeQL query that I used to find the similar value_[n] accesses to the one that caused the bug:

import cpp
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis

from FunctionCall call, ClassTemplateInstantiation t, TemplateClass c, string path
where
  call.getQualifier().(VariableAccess).getTarget().getName() = "value_" and
  call.getTarget().getName() = "operator[]" and
  t = call.getQualifier().getType().getUnspecifiedType() and
  c = t.getTemplate() and
  c.getSimpleName() != "map" and
  not exists (Expr arg |
    c.getSimpleName() = "array" and arg = call.getArgument(0)
    and lowerBound(arg) >= 0 and
    upperBound(arg) < t.getTemplateArgument(1).(Literal).getValue().toInt()) and
  path = call.getLocation().getFile().getRelativePath() and
  not path.matches("xmpsdk/%")
select call, t, c, path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug forward-to-main Forward changes in a 0.28.x PR to main with Mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exiv2 crashes due to assertion '__n < this->size()' failed.
2 participants