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: race condition in TIFF reader #3772

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Feb 13, 2023

TIFFInput::read_native_tiles had a very subtle race condition. There
is a section where it has a task_set of decompression tasks that are
sent to the thread queue. When the method ends, the task_set goes out
of scope, and its destructor will wait for all the remaining tasks to
finish. All fine, right? Except that other variables in the function
include unique_ptr's with allocated memory that the tasks need. So
what can happen is that if the compiler generates the code such that
the destructor of the unique_ptr happens before the destructor of the
task_set, the memory that the unique_ptr held may be deallocated
before some of the tasks still running in threads have finished.

In practice, we seem only to hit this race condition for particular
cases involving corrupted files, because the early return seems
quite likely to run those destructors before all the threads are done,
but it was possible to encounter it for valid files as well, if the
timing is just right. Shocking that we never had it reported.

The solution is just to have an explitit tasks.wait(), which will
block until all the tasks have finished before hitting the actual end
of the method and letting the other destructors to run. And for the
"early return" case, change it to a break so that we go through the
wait call at the end.

I believe that this fix addresses TALOS-2023-1709 / CVE-2023-24472.

Also, in ImageCache, a fix fix where upon tile read error, mark the
file as broken. The idea is to keep fruitlessly reading from a corrupted
file.

The idea is to keep fruitlessly reading from a corrupted file.
TIFFInput::read_native_tiles had a very subtle race condition.  There
is a section where it has a task_set of decompression tasks that are
sent to the thread queue.  When the method ends, the task_set goes out
of scope, and its destructor will wait for all the remaining tasks to
finish. All fine, right? Except that other variables in the function
include unique_ptr's with allocated memory that the tasks need.  So
what can happen is that if the compiler generates the code such that
the destructor of the unique_ptr happens before the destructor of the
task_set, the memory that the unique_ptr held may be deallocated
before some of the tasks still running in threads have finished.

In practice, we seem only to hit this race condition for particular
cases involving corrupted files, because the early `return` seems
quite likely to run those destructors before all the threads are done,
but it was possible to encounter it for valid files as well, if the
timing is just right. Shocking that we never had it reported.

The solution is just to have an explitit tasks.wait(), which will
block until all the tasks have finished before hitting the actual end
of the method and letting the other destructors to run. And for the
"early return" case, change it to a break so that we go through the
wait call at the end.

I believe that this fix addresses TALOS-2023-1709 / CVE-2023-24472.
@lgritz lgritz added this pull request to the merge queue Feb 13, 2023
Merged via the queue into AcademySoftwareFoundation:master with commit f8db9f3 Feb 13, 2023
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Feb 13, 2023
* test: Add a corrupted test file

* fix(IC): upon tile read error, mark the file as broken

The idea is to keep fruitlessly reading from a corrupted file.

* fix: race condition in TIFF reader

TIFFInput::read_native_tiles had a very subtle race condition.  There
is a section where it has a task_set of decompression tasks that are
sent to the thread queue.  When the method ends, the task_set goes out
of scope, and its destructor will wait for all the remaining tasks to
finish. All fine, right? Except that other variables in the function
include unique_ptr's with allocated memory that the tasks need.  So
what can happen is that if the compiler generates the code such that
the destructor of the unique_ptr happens before the destructor of the
task_set, the memory that the unique_ptr held may be deallocated
before some of the tasks still running in threads have finished.

In practice, we seem only to hit this race condition for particular
cases involving corrupted files, because the early `return` seems
quite likely to run those destructors before all the threads are done,
but it was possible to encounter it for valid files as well, if the
timing is just right. Shocking that we never had it reported.

The solution is just to have an explitit tasks.wait(), which will
block until all the tasks have finished before hitting the actual end
of the method and letting the other destructors to run. And for the
"early return" case, change it to a break so that we go through the
wait call at the end.

I believe that this fix addresses TALOS-2023-1709 / CVE-2023-24472.
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Feb 13, 2023
* test: Add a corrupted test file

* fix(IC): upon tile read error, mark the file as broken

The idea is to keep fruitlessly reading from a corrupted file.

* fix: race condition in TIFF reader

TIFFInput::read_native_tiles had a very subtle race condition.  There
is a section where it has a task_set of decompression tasks that are
sent to the thread queue.  When the method ends, the task_set goes out
of scope, and its destructor will wait for all the remaining tasks to
finish. All fine, right? Except that other variables in the function
include unique_ptr's with allocated memory that the tasks need.  So
what can happen is that if the compiler generates the code such that
the destructor of the unique_ptr happens before the destructor of the
task_set, the memory that the unique_ptr held may be deallocated
before some of the tasks still running in threads have finished.

In practice, we seem only to hit this race condition for particular
cases involving corrupted files, because the early `return` seems
quite likely to run those destructors before all the threads are done,
but it was possible to encounter it for valid files as well, if the
timing is just right. Shocking that we never had it reported.

The solution is just to have an explitit tasks.wait(), which will
block until all the tasks have finished before hitting the actual end
of the method and letting the other destructors to run. And for the
"early return" case, change it to a break so that we go through the
wait call at the end.

I believe that this fix addresses TALOS-2023-1709 / CVE-2023-24472.
@lgritz lgritz deleted the lg-tiffrace2 branch February 13, 2023 16:40
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Mar 8, 2023
* test: Add a corrupted test file

* fix(IC): upon tile read error, mark the file as broken

The idea is to keep fruitlessly reading from a corrupted file.

* fix: race condition in TIFF reader

TIFFInput::read_native_tiles had a very subtle race condition.  There
is a section where it has a task_set of decompression tasks that are
sent to the thread queue.  When the method ends, the task_set goes out
of scope, and its destructor will wait for all the remaining tasks to
finish. All fine, right? Except that other variables in the function
include unique_ptr's with allocated memory that the tasks need.  So
what can happen is that if the compiler generates the code such that
the destructor of the unique_ptr happens before the destructor of the
task_set, the memory that the unique_ptr held may be deallocated
before some of the tasks still running in threads have finished.

In practice, we seem only to hit this race condition for particular
cases involving corrupted files, because the early `return` seems
quite likely to run those destructors before all the threads are done,
but it was possible to encounter it for valid files as well, if the
timing is just right. Shocking that we never had it reported.

The solution is just to have an explitit tasks.wait(), which will
block until all the tasks have finished before hitting the actual end
of the method and letting the other destructors to run. And for the
"early return" case, change it to a break so that we go through the
wait call at the end.

I believe that this fix addresses TALOS-2023-1709 / CVE-2023-24472.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant