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

Fix out of bounds copy in LoadBorders() #775

Merged
merged 1 commit into from Oct 26, 2021
Merged

Conversation

deymo
Copy link
Contributor

@deymo deymo commented Oct 26, 2021

When processing groups out of order with an image where the last group
size is smaller than the needed border it was possible to attempt to
load the right or bottom border from the next group when already
processing the rightmost or bottom group respectively. This situation
was causing an out-of-bounds copy on saved Image3F buffer in release
mode (or hitting a JXL_DASSERT in debug mode).

The order in which the groups are processed depends on many factors,
including the order in which the threads are scheduled when using
multiple threads, and potentially the order of the groups in the file
(not checked).

Added a test to force the out-of-order situation in a simulated parallel
runner that forces a random order on the tasks. The new test triggers
the assert in debug mode, which is now fixed.

Fixes #708.

@deymo deymo requested a review from veluca93 October 26, 2021 18:11
lib/jxl/jxl_test.cc Outdated Show resolved Hide resolved
lib/jxl/jxl_test.cc Outdated Show resolved Hide resolved
veluca93
veluca93 previously approved these changes Oct 26, 2021
veluca93
veluca93 previously approved these changes Oct 26, 2021
veluca93
veluca93 previously approved these changes Oct 26, 2021
When processing groups out of order with an image where the last group
size  is smaller than the needed border it was possible to attempt to
load the right or bottom border from the next group when already
processing the rightmost or bottom group respectively. This situation
was causing an out-of-bounds copy on saved Image3F buffer in release
mode (or hitting a JXL_DASSERT in debug mode).

The order in which the groups are processed depends on many factors,
including the order in which the threads are scheduled when using
multiple threads, and potentially the order of the groups in the file
(not checked).

Added a test to force the out-of-order situation in a simulated parallel
runner that forces a random order on the tasks. The new test triggers
the assert in debug mode, which is now fixed.

Fixes libjxl#708.
@deymo deymo merged commit e649705 into libjxl:main Oct 26, 2021
18 checks passed
@deymo deymo deleted the thread_bug branch October 26, 2021 23:25
@deymo deymo added the merge-0.6 PR label to flag PRs that need to be merged to v0.6.x label Oct 27, 2021
deymo added a commit to deymo/libjxl that referenced this pull request Oct 27, 2021
When processing groups out of order with an image where the last group
size  is smaller than the needed border it was possible to attempt to
load the right or bottom border from the next group when already
processing the rightmost or bottom group respectively. This situation
was causing an out-of-bounds copy on saved Image3F buffer in release
mode (or hitting a JXL_DASSERT in debug mode).

The order in which the groups are processed depends on many factors,
including the order in which the threads are scheduled when using
multiple threads, and potentially the order of the groups in the file
(not checked).

Added a test to force the out-of-order situation in a simulated parallel
runner that forces a random order on the tasks. The new test triggers
the assert in debug mode, which is now fixed.

Fixes libjxl#708.
(cherry picked from commit e649705)
(cherry picked from PR libjxl#775)
deymo added a commit to deymo/libjxl that referenced this pull request Oct 27, 2021
When processing groups out of order with an image where the last group
size  is smaller than the needed border it was possible to attempt to
load the right or bottom border from the next group when already
processing the rightmost or bottom group respectively. This situation
was causing an out-of-bounds copy on saved Image3F buffer in release
mode (or hitting a JXL_DASSERT in debug mode).

The order in which the groups are processed depends on many factors,
including the order in which the threads are scheduled when using
multiple threads, and potentially the order of the groups in the file
(not checked).

Added a test to force the out-of-order situation in a simulated parallel
runner that forces a random order on the tasks. The new test triggers
the assert in debug mode, which is now fixed.

(cherry picked from commit e649705)
(cherry picked from PR libjxl#775)

Adapted the cherry-pick to use <random> in the test instead of
"lib/jxl/random.h" since the latter is not available in v0.6.x branch.
deymo added a commit to deymo/libjxl that referenced this pull request Oct 27, 2021
When processing groups out of order with an image where the last group
size  is smaller than the needed border it was possible to attempt to
load the right or bottom border from the next group when already
processing the rightmost or bottom group respectively. This situation
was causing an out-of-bounds copy on saved Image3F buffer in release
mode (or hitting a JXL_DASSERT in debug mode).

The order in which the groups are processed depends on many factors,
including the order in which the threads are scheduled when using
multiple threads, and potentially the order of the groups in the file
(not checked).

Added a test to force the out-of-order situation in a simulated parallel
runner that forces a random order on the tasks. The new test triggers
the assert in debug mode, which is now fixed.

(cherry picked from commit e649705)
(cherry picked from PR libjxl#775)

Adapted the cherry-pick to use <random> in the test instead of
"lib/jxl/random.h" since the latter is not available in v0.6.x branch.
deymo added a commit that referenced this pull request Oct 27, 2021
When processing groups out of order with an image where the last group
size  is smaller than the needed border it was possible to attempt to
load the right or bottom border from the next group when already
processing the rightmost or bottom group respectively. This situation
was causing an out-of-bounds copy on saved Image3F buffer in release
mode (or hitting a JXL_DASSERT in debug mode).

The order in which the groups are processed depends on many factors,
including the order in which the threads are scheduled when using
multiple threads, and potentially the order of the groups in the file
(not checked).

Added a test to force the out-of-order situation in a simulated parallel
runner that forces a random order on the tasks. The new test triggers
the assert in debug mode, which is now fixed.

(cherry picked from commit e649705)
(cherry picked from PR #775)

Adapted the cherry-pick to use <random> in the test instead of
"lib/jxl/random.h" since the latter is not available in v0.6.x branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-0.6 PR label to flag PRs that need to be merged to v0.6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash during multiple concurrent/parallel decoding
2 participants