The input buffer must be valid as long as the main thread is writing
to the worker-specific input buffer. Fix it by making the worker
thread not free the buffer on errors and not return the worker thread to
the pool. The input buffer will be freed when threads_end() is called.
With invalid input, the bug could at least result in a crash. The
effects include heap use after free and writing to an address based
on the null pointer plus an offset.
The bug has been there since the first committed version of the threaded
decoder and thus affects versions from 5.3.3alpha to 5.8.0.
As the commit message in 4cce3e27f529 says, I had made significant
changes on top of Sebastian's patch. This bug was indeed introduced
by my changes; it wasn't in Sebastian's version.
Thanks to Harri K. Koskinen for discovering and reporting this issue.
Fixes: 4cce3e27f529 ("liblzma: Add threaded .xz decompressor.")
Reported-by: Harri K. Koskinen <x64nop@nannu.org>
Reviewed-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Thanks-to: Sam James <sam@gentoo.org>
The main thread can directly set THR_IDLE in threads_stop() which is
called when errors are detected. threads_stop() won't return the stopped
threads to the pool or free the memory pointed by thr->in anymore, but
it doesn't matter because the existing workers won't be reused after
an error. The resources will be cleaned up when threads_end() is
called (reinitializing the decoder always calls threads_end()).
Reviewed-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Thanks-to: Sam James <sam@gentoo.org>
This is similar to 2ce4f36f179a81d0c6e182a409f363df759d1ad0.
The actual initialization of the variables is done inside
mythread_sync() macro. Clang doesn't seem to see that
the initialization code inside the macro is always executed.
This affects lzma_memusage() and lzma_memlimit_set() when used
with the threaded decompressor. Now all allocations are reported
by lzma_memusage() (so it's not misleading) and lzma_memlimit_set()
cannot lower the limit below that value.
The alternative would have been to allow lowering the limit if
doing so is possible by freeing the cached memory but since
the primary use case of lzma_memlimit_set() is to increase
memlimit after LZMA_MEMLIMIT_ERROR this simple approach
was selected.
The cached memory was always included when enforcing
the memory usage limit while decoding.
Thanks to Jia Tan.
It will now return LZMA_DATA_ERROR (not LZMA_OK or LZMA_BUF_ERROR)
if LZMA_FINISH is used and there isn't enough input to finish
decoding the Block Header or the Block. The use of LZMA_DATA_ERROR
is simpler and the less risky than LZMA_BUF_ERROR but this might
be changed before 5.4.0.
In most cases if the input file is corrupt the application won't
care about the uncompressed content at all. With this new flag
the threaded decoder will return an error as soon as any thread
has detected an error; it won't wait to copy out the data before
the location of the error.
I don't plan to use this in xz to keep the behavior consistent
between single-threaded and multi-threaded modes.
This makes it possible to call lzma_code() in a loop that only
reads new input when lzma_code() didn't fill the output buffer
completely. That isn't the calling style suggested by the
liblzma example program 02_decompress.c so perhaps the usefulness
of this feature is limited.
Also, it is possible to write such a loop so that it works
with the single-threaded decoder but not with the threaded
decoder even after this commit, or so that it works only if
lzma_mt.timeout = 0.
The zlib tutorial <https://zlib.net/zlib_how.html> is a well-known
example of a loop where more input is read only when output isn't
full. Porting this as is to liblzma would work with the
single-threaded decoder (if LZMA_CONCATENATED isn't used) but it
wouldn't work with threaded decoder even after this commit because
the loop assumes that no more output is possible when it cannot
read more input ("if (strm.avail_in == 0) break;"). This cannot
be fixed at liblzma side; the loop has to be modified at least
a little.
I'm adding this in any case because the actual code is simple
and short and should have no harmful side-effects in other
situations.
If a worker thread has consumed all input so far and it's
waiting on thr->cond and then the main thread enables
partial update for that thread, the code used to deadlock.
This commit allows one dummy decoding pass to occur in this
situation which then also does the partial update.
As part of the fix, this moves thr->progress_* updates to
avoid the second thr->mutex locking.
Thanks to Jia Tan for finding, debugging, and reporting the bug.
LZMA_TIMED_OUT is not an error and thus stopping threads on
LZMA_TIMED_OUT breaks the decoder badly.
Thanks to Jia Tan for finding the bug and for the patch.
I realize that this is about a decade late.
Big thanks to Sebastian Andrzej Siewior for the original patch.
I made a bunch of smaller changes but after a while quite a few
things got rewritten. So any bugs in the commit were created by me.
