Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
hassec merged 4 commits into from
Jun 26, 2021
Merged

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

hassec merged 4 commits into from
Jun 26, 2021

Conversation

@kevinbackhouse
Copy link
Collaborator

@kevinbackhouse kevinbackhouse commented Jun 21, 2021

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
exiv2 crashes due to assertion '__n < this->size()' failed. #1706
Closed
hassec
hassec reviewed Jun 22, 2021
View reviewed changes
hassec
hassec reviewed Jun 22, 2021
View reviewed changes
hassec
hassec approved these changes Jun 25, 2021
View reviewed changes
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
@kevinbackhouse
fix: use vector::at() rather than operator[] (#1735)
3c81b18 
* 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
@mergify mergify bot mentioned this pull request Jun 26, 2021
Merged
@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
@mergify @kevinbackhouse @hassec
fix: use vector::at() rather than operator[] (backport #1735) (#1741)
1d64f48 
* 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 kevinbackhouse mentioned this pull request Jul 11, 2021
Merged
@kevinbackhouse
Copy link
Collaborator Author

kevinbackhouse commented Jul 11, 2021

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

@clanmills clanmills mentioned this pull request Aug 9, 2021
Open
@nehaljwani nehaljwani mentioned this pull request May 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hassec hassec hassec approved these changes

Assignees

No one assigned

Labels

bug forward-to-main Forward changes in a 0.28.x PR to main with Mergify

Projects

None yet

Milestone

v0.27.5

Development

Successfully merging this pull request may close these issues.

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

2 participants

@kevinbackhouse @hassec