Cyan4973 / FiniteStateEntropy

New generation entropy codecs : Finite State Entropy and Huff0
BSD 2-Clause "Simplified" License
1.33k stars 143 forks source link

NULL pointer dereference in BIT_reloadDStream() #96

Open JohanKnutzen opened 5 years ago

JohanKnutzen commented 5 years ago

Crashing line: 430: bitD->bitContainer = MEM_readLEST(bitD->ptr); in bitstream.h in the function BIT_reloadDStream() triggered by FSE_decompressU16() with the following code:

const uint8_t* casted2 = (uint8_t*)"Àâ(¢x(Kùÿÿcb¿\a";
uint16_t out[256];
size_t ret = FSE_decompressU16(out, 256, casted2, 14);

This is caused by size_t const NSize = FSE_readNCount (NCount, &maxSymbolValue, &tableLog, istart, cSrcSize); returning zero in the following block of FSE_decompressU16():

{ size_t const NSize = FSE_readNCount (NCount, &maxSymbolValue, &tableLog, istart, cSrcSize); if (FSE_isError(NSize)) return NSize; ip += NSize; cSrcSize -= NSize; } thus, resulting in a zero cSrcSize when entering FSE_decompressU16_usingDTable() which is unexpected

I don't know if this can happen in the 8 bit version.

JohanKnutzen commented 4 years ago

Is this fixed in dev? I can test this one as well if it is believed to be fixed

Cyan4973 commented 4 years ago

There have been a number of changes in fse since 2019, but I'm not sure if one of them addresses this concern. FSE_readNCount was updated in zstd, and maybe it fixed this issue. Note though that not all updates are sync into fse repository yet : we do have difficulties to keep both repositories automatically synchronized, for complex path & namespace reasons, so it always requires some manual work to do that.

JohanKnutzen commented 3 years ago

@Cyan4973 Sorry for the late reply. I just updated to the latest version of fse (dev branch) and it still crashes. The repro is still

const uint8_t* casted2 = (uint8_t*)"Àâ(¢x(Kùÿÿcb¿\a";
uint16_t out[256];
size_t ret = FSE_decompressU16(out, 256, casted2, 14);

I have a more complex repro that involves FSE_compressU16 if you'd like me to provide it.

JohanKnutzen commented 3 years ago

@Cyan4973 Bump!

Cyan4973 commented 3 years ago

I'm sorry, there is currently no availability left to continue maintaining FSE as a separate library.

There is ongoing work on FSE, but as part of zstd, and these code bases are now diverging. And for example, the FSE_compressU16() variant is not used there, so is not supported.

The situation could be different in some future, but in the foreseeable future, I'm afraid I can't offer the time for it.

JohanKnutzen commented 3 years ago

Alright, thanks @Cyan4973

If I find a fix for this, can I do a PR on this repo?

Cyan4973 commented 3 years ago

Yes