Fix null-pointer dereferences and partial-init failure - #65
Conversation
90b10ac to
5ffeaa2
Compare
- LOAD_SYMBOL no longer returns early on failure. Instead it sets a global 'shim_disabled' flag and continues resolving all remaining symbols. This prevents null function-pointer calls when one symbol fails to load while others are still interposed. - Add null checks for pointers returned by ShardedMap::Get() in deflate(), inflate(), deflateSetDictionary(), gzwrite(), gzread(), gzclose(), gzeof(), GetDeflateExecutionPath(), and GetInflateExecutionPath(). When the pointer is null (e.g. stream opened via unintercepted gzopen64/deflateCopy, or shim disabled), fall through to the corresponding orig_* function. Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
deflateSetDictionary got a null check for its ShardedMap::Get() result but inflateSetDictionary did not, though it is reachable the same way. Registry entries are only created in inflateInit_/inflateInit2_, so a stream that became valid through an unintercepted entry point -- inflateCopy() is the simplest -- is never registered and Get() returns null. Reproduced on main as a SIGSEGV inside the shim, for two legal call sequences that both work under plain zlib: raw inflate, where inflateSetDictionary may be called at any time, and the wrapped case where inflate() must first return Z_NEED_DICT. With the guard both complete and match plain zlib byte for byte. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
5ffeaa2 to
14583a6
Compare
| @@ -275,7 +278,9 @@ int ZEXPORT deflateSetDictionary(z_streamp strm, const Bytef* dictionary, | |||
| Log(LogLevel::LOG_INFO, "deflateSetDictionary Line ", __LINE__, ", strm ", | |||
| static_cast<void*>(strm), ", dictLength ", dictLength, "\n"); | |||
| auto deflate_settings = deflate_stream_settings.Get(strm); | |||
There was a problem hiding this comment.
Added a guard for inflateSetDictionary (new commit)
deflateSetDictionary got a null check but inflateSetDictionary did not, and it is
reachable identically.
There was a problem hiding this comment.
make shim_disabled per-symbol rather than global
The shim_disabled half needs rework:
-
The fallback dereferences the pointer that may be null.
if (... || shim_disabled) return orig_deflate(...). iforig_deflateis the symbol that failed, this calls a null pointer, i.e. the crash the change is meant to prevent. The condition tests a global meaning "something failed" while dereferencing a specific pointer that was never validated. All 16return orig_*sites share this shape. -
One failure disables everything. 23
LOAD_SYMBOLcalls feed one flag. Concretely:uncompress2is version-scoped (uncompress2@@ZLIB_1.2.9) and doesn't exist before zlib 1.2.9, so on an older-but-valid zlibdlsymreturns null for it while everything else resolves — and acceleration is silently switched off for all ofdeflate/inflate/gzwrite. Same forgzopen64(needs 1.2.3.3).
Suggested shape: drop the global, check each orig_* at its own call site, so a missing
symbol disables exactly the path that needs it. That still leaves what to do when the needed pointer is null — the shim has no implementation of its own, so a missing orig_deflate is unrecoverable and better handled loudly at init than silently at call time.
2 participants
LOAD_SYMBOL no longer returns early on failure. Instead it sets a global 'shim_disabled' flag and continues resolving all remaining symbols. This prevents null function-pointer calls when one symbol fails to load while others are still interposed.
Add null checks for pointers returned by ShardedMap::Get() in deflate(), inflate(), deflateSetDictionary(), gzwrite(), gzread(), gzclose(), gzeof(), GetDeflateExecutionPath(), and GetInflateExecutionPath(). When the pointer is null (e.g. stream opened via unintercepted gzopen64/deflateCopy, or shim disabled), fall through to the corresponding orig_* function.