laudrup / lz4_stream Goto Github PK

first of all, thanks for the help, this makes working with LZ4 more bearable.

Second, I tried using the input stream on the output of a large json filled file(I can't attach it since it was client's access logs) and discovered the output is getting truncated after about 14k.

I tracked the error down to the following line:
if (dest_size == 0)

basically, lz4 doesn't HAVE to decompress a full block on this command, in some case the decompress function will return 0 decompressed because it didn't receive enough data to push anything out.

thus this if will EOF the stream prematurely.

What worked for me(extracted the whole file and the md5sum of the result was equal to lz4cat's) was to copy the example from LZ4's code
https://github.com/lz4/lz4/blob/dev/examples/frameCompress.c

Here's the PR with my code:
#1

I don't have much experience on C/CPP, so I probably made some mistakes, but this might help you find the issue

I'm new'ish to cmake, certainly so on Windows. But I have a VS2022 Cmake console app running. It is unclear to me how to setup the lz4 library (which I've downloaded and added the include/lib paths to my CMakeList.txt). Also, I've setup my git path and attempted to use set(LZ4_STREAM_UseBundledLz4 TRUE). However, I cannot get it to work.

I get an error about needing:
lz4_stream/thirdparty/lz4/LibLz4-prefix/src/LibLz4/visual/VS2010/bin/_Debug/liblz4_static.lib

Hi, thanks for your project!

I considered using it but decided to pipe out to the lz4 utility instead (for now). Yet, I wanted to write downs some points so that the time I spent reading its code does not go unused.

(I maintain the Haskell binding to lz4frame.h and thus am familiar with the fact that the lz4 API can be tricky, and I'm also interested in double-checking whether my understanding is correct to improve my own library if they aren't.)

These are the points/questions:

1. Potential memory leak:
   The constructor output_buffer(std::ostream &sink) seems to be able to throw in the last function it calls, write_header();. When that happens, LZ4F_createCompressionContext() will have malloc'ed a ctx_ and the destructor will not be called to free it. Now I'm not very sure whether write_header() will throw beyond the explicit throw that the library does, which should not happen unless there's a bug. Is it possible for sink_.write() to throw in any situation, or will failures (such as bad_alloc, disk full, TCP client closed the connection of the downstream stream) always be silent?
   In any case, it may make sense to make a guaranteed non-throwing write_header() for use in the constructor, and ensure all control flow paths out of the constructor past the ctx_ creation call LZ4F_freeCompressionContext(ctx_).
2. The implementation seems to do a multiple function calls and a stream yield for each incoming byte (overflow() uses pbump(1)), which seems extremely inefficient given that overflow() is virtual. Is this correct, or am I missing something?
3. The 256 bytes buffer by default seems quite small. What motivated that value?
4. In this line

   lz4_stream/include/lz4_stream.h

   Line 120 in 6b015cb

   sink_.write(&dest_buf_.front(), ret);

   it's using sink_.write() even though the output buffer may be empty (somewhat inefficient), because the lz4 context has an internal buffer. Would it make sene to skip the write() in that case?
5. In this line

   lz4_stream/include/lz4_stream.h

   Line 65 in 6b015cb

   dest_buf_(LZ4F_compressBound(src_buf_.size(), nullptr)),

   it might be beneficial to + LZ4F_HEADER_SIZE_MAX for efficiency, to ensure both the header and the first chunk fit into the allocated buffer (like I do in Haskell here).
6. The examples like lz4_compress.cpp don't do any error handling, like complaining when the input file does not exist or when the output file cannot be written to. They exhibit somewhat funny behaviour, such as that ./lz4_compress nonexistent outfile generates some bytes into out even though the input file does not exist, and it starts reading the input file even after the openat() syscall has already failed (checked with strace). While that's not a real problem, it can be confusing if you're playing the the example and thinking that it's working, when silent failure is actually happening.

Thanks for taking a look!

hello,

In (master) CMakeLists you use LZ4_INCLUDE_DIR but pkg_config defines LZ4_INCLUDEDIR ...
this breaks the case in which you don't want to use the supplied lz4.

in another note: i'm trying to crosscompile, and pkg_config also gives a library directory which is not being taken into account, but that's not so simple to fix as target_link_directories only exists in very recent versions of CMake. i work around by symlinking liblz4.a.

Frank

Unit testing library "Catch2" throws error on OS:
amazonlinux:2022

Compiler (OS Default):
g++ (GCC) 11.2.1 20210728 (Red Hat 11.2.1-2)

Error can be avoided by disabling the tests with:
cmake -DLZ4_STREAM_BuildTests=OFF ..

(It's referring to "MINSIGSTKSZ" as being non-constant)

Example Error output:
[ 80%] Completed 'Catch2' [ 80%] Built target Catch2 [ 86%] Building CXX object test/CMakeFiles/lz4_stream_test.dir/main.cpp.o [ 93%] Building CXX object test/CMakeFiles/lz4_stream_test.dir/lz4_stream_test.cpp.o In file included from /usr/include/signal.h:328, from /build_dep/src/lz4_stream/build/thirdparty/catch2/Catch2-prefix/src/Catch2/single_include/catch2/catch.hpp:5903, from /build_dep/src/lz4_stream/test/main.cpp:2: /build_dep/src/lz4_stream/build/thirdparty/catch2/Catch2-prefix/src/Catch2/single_include/catch2/catch.hpp:8525:58: error: call to non-'constexpr' function 'long int sysconf(int)' 8525 | constexpr static std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ; | ^~~~~~~~~~~ In file included from /usr/include/bits/sigstksz.h:24, from /usr/include/signal.h:328, from /build_dep/src/lz4_stream/build/thirdparty/catch2/Catch2-prefix/src/Catch2/single_include/catch2/catch.hpp:5903, from /build_dep/src/lz4_stream/test/main.cpp:2: /usr/include/unistd.h:640:17: note: 'long int sysconf(int)' declared here 640 | extern long int sysconf (int __name) __THROW; | ^~~~~~~ In file included from /build_dep/src/lz4_stream/test/main.cpp:2: /build_dep/src/lz4_stream/build/thirdparty/catch2/Catch2-prefix/src/Catch2/single_include/catch2/catch.hpp:8584:45: error: size of array 'altStackMem' is not an integral constant-expression 8584 | char FatalConditionHandler::altStackMem[sigStackSize] = {}; | ^~~~~~~~~~~~ make[2]: *** [test/CMakeFiles/lz4_stream_test.dir/build.make:76: test/CMakeFiles/lz4_stream_test.dir/main.cpp.o] Error 1 make[1]: *** [CMakeFiles/Makefile2:198: test/CMakeFiles/lz4_stream_test.dir/all] Error 2 make: *** [Makefile:101: all] Error 2