ZSTD_decompressStream checks its state parameter is not NULL

[GitMensch]

preventing segmentation faults seen with compressed perf recordings

perf record --aio -z -- true && perf annotate

Stacktrace from Debian, nearly identical in RHEL.

Program received signal SIGSEGV, Segmentation fault.
bZSTD_decompressStream (zds=0x0, output=output@entry=0x7fffffffc060, input=input@entry=0x7fffffffc040) at decompress/zstd_decompress.c:1657
1657    decompress/zstd_decompress.c: No such file or directory.
(gdb) bt
#0  ZSTD_decompressStream (zds=0x0, output=output@entry=0x7fffffffc060, input=input@entry=0x7fffffffc040) at decompress/zstd_decompress.c:1657
#1  0x00005555558dd2dc in zstd_decompress_stream (data=data@entry=0x555555de4898, src=src@entry=0x7ffff7ffb248, src_size=src_size@entry=389, dst=0x7ffff665a028,
    dst_size=dst_size@entry=528384) at util/zstd.c:100
#2  0x000055555583ea8d in perf_session__process_compressed_event (session=0x555555dddc10, event=0x7ffff7ffb240, file_offset=<optimized out>) at util/session.c:74
#3  0x00005555558409c1 in perf_session__process_user_event (session=session@entry=0x555555dddc10, event=event@entry=0x7ffff7ffb240, file_offset=file_offset@entry=576)
    at util/session.c:1664
#4  0x000055555584242b in perf_session__process_event (file_offset=576, event=0x7ffff7ffb240, session=0x555555dddc10) at util/session.c:1797
#5  0x0000555555843afa in reader__process_events (prog=0x7fffffffc720, session=0x555555dddc10, rd=<synthetic pointer>) at util/session.c:2251
#6  __perf_session__process_events (session=0x555555dddc10) at util/session.c:2309
#7  perf_session__process_events (session=session@entry=0x555555dddc10) at util/session.c:2342
#8  0x000055555574991d in __cmd_annotate (ann=0x7fffffffc880) at builtin-annotate.c:402
#9  cmd_annotate (argc=<optimized out>, argv=0x7fffffffdc40) at builtin-annotate.c:647
#10 0x00005555557ddb43 in run_builtin (p=p@entry=0x555555bb5168 <commands+360>, argc=argc@entry=1, argv=argv@entry=0x7fffffffdc40) at perf.c:313
#11 0x0000555555747c08 in handle_internal_command (argv=0x7fffffffdc40, argc=1) at perf.c:365
#12 run_argv (argv=<synthetic pointer>, argcp=<synthetic pointer>) at perf.c:409
#13 main (argc=1, argv=0x7fffffffdc40) at perf.c:539

Likely perf is to be blamed in the first place, but as there are already checks for other issues it seems to be useful to cater for that.

[facebook-github-bot]

Hi @GitMensch!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[GitMensch]

CLA signed

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[Cyan4973]

I would change the title, because it is misleading.
What you mean is that ZSTD_decompressStream() should check that the state pointer is not NULL.

Indeed, such a scenario is a major breach of contract, it points to a serious error on the user side.

So far, this scenario has been classified as a "breach of contract", meaning we just document the (obvious) convention that ZSTD_DStream* must be valid, but if it's not, it's UB (Undefined Behavior). For perspective, same thing happens if the pointer is not NULL but completely bogus (dangling).

We could nonetheless debate about extending the contract to make ZSTD_decompressStream() able to detect a NULL state and cleanly return an error in this case. For this case, the error should be clearly documented (I suspect no corresponding error code exist yet, and stage_wrong is a too rough approximation of the problem, that can be misleading for debugging).

What I'm concerned though is about consistency: if we do that for this symbol, we would have to do that for every other symbol which manipulates a state as head parameter, for consistency sake. And there are really a lot of them.

[Cyan4973]

We'll preserve the current contract,
which requires that the state pointer must be valid.
Therefore, this should be an assert() and not a test.

[Cyan4973]

replaces by #4249