DPX output: fix write errors #3672
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a subtle error where we has a reference to an ImageSpec for one of the subimages, confusingly called m_spec, which we altered, but then didn't make the same change to the this->m_spec, and in a later spot we referenced the `m_spec` meaning the one in this. This could lead to a crash. Fix, but not only that, rename to avoid the confusion. `spec0` is the alias we us for the spec for subimage 0, and `spec_s` is the alias we use when referencing subimage N, and we really never use m_spec anymore to avoi any confusion. This should make it easier to reason about this code in the future and avoid thise kinds of mistakes. A second error resulted from the situation where a failure in open() would return before one or both of m_buf or m_tilebuf were allocated. If the return status of open() were ignored and the caller subsequently attempted calls to write_scanline or write_tile, invalid memory could be accessed. Fix this with a variety of guards not only based on the allocation status of these buffers, but also issuing errors (and exiting early) from write_scanline or write_tile if they are called on an ImageInput that is not in fact open. (Addresses 1651, 1652)
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Nov 20, 2022
There was a subtle error where we had a reference to an ImageSpec for one of the subimages, confusingly called m_spec, which we altered, but then didn't make the same change to the this->m_spec, and in a later spot we referenced the `m_spec` meaning the one in `this`. This could lead to a crash. Fix, but not only that, also rename to avoid the confusion. `spec0` is the alias we us for the spec for subimage 0, and `spec_s` is the alias we use when referencing subimage S, and we really never use m_spec anymore to avoid any confusion. This should make it easier to reason about this code in the future and avoid these kinds of mistakes. A second error resulted from the situation where a failure in open() would return before one or both of m_buf or m_tilebuf were allocated. If the return status of open() were ignored and the caller subsequently attempted calls to write_scanline or write_tile, invalid memory could be accessed. Fix this with a variety of guards not only based on the allocation status of these buffers, but also issuing errors (and exiting early) from write_scanline or write_tile if they are called on an ImageInput that is not in fact open. Addresses TALOS-2022-1651 / CVE-2022-43592 and TALOS-2022-1652 / CVE-2022-43593
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a subtle error where we has a reference to an ImageSpec for one of the subimages, confusingly called m_spec, which we altered, but then didn't make the same change to the this->m_spec, and in a later spot we referenced the
m_specmeaning the one in this. This could lead to a crash.Fix, but not only that, rename to avoid the confusion.
spec0is the alias we us for the spec for subimage 0, andspec_sis the alias we use when referencing subimage N, and we really never use m_spec anymore to avoi any confusion. This should make it easier to reason about this code in the future and avoid thise kinds of mistakes.A second error resulted from the situation where a failure in open() would return before one or both of m_buf or m_tilebuf were allocated. If the return status of open() were ignored and the caller subsequently attempted calls to write_scanline or write_tile, invalid memory could be accessed. Fix this with a variety of guards not only based on the allocation status of these buffers, but also issuing errors (and exiting early) from write_scanline or write_tile if they are called on an ImageInput that is not in fact open.
(Addresses 1651, 1652)