Fix potential context leak, add exceptions, add lz4-style program for testing #6
Conversation
The lz4 docs do not specify whether freeing multiple times is a no-op or legal, so we better check the safer invariant.
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
==========================================
- Coverage 90.62% 90.51% -0.12%
==========================================
Files 2 2
Lines 128 137 +9
==========================================
+ Hits 116 124 +8
- Misses 12 13 +1
Continue to review full report at Codecov.
|
include/lz4_stream.h
Outdated
| /** | ||
| * @brief RAII version of `LZ4F_compressionContext_t`. | ||
| */ | ||
| struct CompressionContext |
There was a problem hiding this comment.
Minor thing. The other classes use snake_case naming. I don't have any strong feelings one way or the other, but I really like to be consistent.
| size_t ret = LZ4F_createCompressionContext(&ctx, LZ4F_VERSION); | ||
| if (LZ4F_isError(ret) != 0) { | ||
| throw std::runtime_error(std::string("Failed to create LZ4 compression context: ") | ||
| + LZ4F_getErrorName(ret)); |
There was a problem hiding this comment.
Unrelated to this change, but reminds me that I have thought or creating a lz4_category that inherits from std::error_category and the throw std::system_error instead.
That way a more specific exception could be thrown as well as not having to add the LZ4 error string every time an exception object is constructed. Makes sense?
There was a problem hiding this comment.
Yes, I think specific exceptions are good so that they can be caught explicitly.
I'll leave that to you though :)
| @@ -62,3 +62,6 @@ target_link_libraries(lz4_compress ${LZ4_STREAM_LIBRARY_NAME}) | |||
|
|
|||
| add_executable(lz4_decompress lz4_decompress.cpp) | |||
| target_link_libraries(lz4_decompress ${LZ4_STREAM_LIBRARY_NAME}) | |||
|
|
|||
| add_executable(lz4_dropin lz4_dropin.cpp) | |||
| target_link_libraries(lz4_dropin ${LZ4_STREAM_LIBRARY_NAME}) | |||
There was a problem hiding this comment.
Feel free to remove the existing example executables once this compiles.
Example case: The `output_buffer()` constructor can throw via `write_header()`. Until now, the malloc'ed context would then not be freed (because if the constructor throws, no destructor is called). This commit fixes it by wrapping contexts in RAII classes that always ensure destruction.
Motivating example:
Until now, the example `lz4_compress` application would silently
ignore errors like `no space left on device` (ENOSP) when compressing
continuing to churn through the input data and writing to the
output file even though every write fails (as evidenced by `strace`)
and the resulting .lz4 file is naturally corrupted.
To reproduce:
mkdir ramdisk
sudo mount -t tmpfs -o size=1m tmpfs ramdisk/
./lz4_compress /some/big/file ramdisk/out.lz4
Discussion:
C++ iostreams were invented before exceptions which is why they are
disabled by default and have to be opted in for each stream.
If exceptions are not enabled, the badbit should be checked before/after
every stream operation. If that is not done, errors go silently
and the user marvels over odd syscall behaviour and corrupt data.
This library does not check the badbit.
This commit fixes the situation it by enabling exceptions using
stream.exceptions(std::ostream::failbit);
thus enforcing what the library's current assumption that streams
are not in an invalid state.
Further work:
An alternative approach might be to change the library to get rid
of that assumption and switch to the badbit-check everywhere,
but this would be more difficult. It might present good follow-up work
to this fix.
As a reminder:
An application might want to handle ENOSP by looping until more
space is available on the file system, and re-use the open LZ4
stream as soon as that is the case.
This commit still does not make that use case possible, because
if the user catches the exception and calls the library again,
the `input_buffer`/`output_buffer` would be advanced (e.g. by more
LZ4 operations), so the stream would not be valid.
A fix for that would be to observe any stream errors in the library,
and ensure that when the user calls it again, the existing buffers
are not invalidated.
nh2
force-pushed
the
fix-context-leak-add-exceptions
branch
from
c1210cb
to
62a5ec0
Compare
|
I've force-pushed to address the feedback. |
nh2
force-pushed
the
fix-context-leak-add-exceptions
branch
from
62a5ec0
to
adf65aa
Compare
|
I've force-pushed a rework of the It can now accept Examples: I did this because it makes it easier to run the example application through |
See #5 and the commit messages.