Add DDS to oss fuzzer

[senhuang42]

DDS wasn't getting fuzzed, this adds it (back?) to OSS-Fuzz

[senhuang42]

Looks like something is not quite happy with how DDS adjusts its parameters and the increase and/or decrease of hashLog.

[senhuang42]

Also, local testing reveals that this assert is also failing make -j regressiontest:

zstd/lib/compress/zstd_lazy.c

Line 531 in 3eb3845

assert(chainPos <= chainSize); /* I believe this is guaranteed... */

[Cyan4973]

Also, local testing reveals that this assert is also failing make -j regressiontest:

zstd/lib/compress/zstd_lazy.c

Line 531 in 3eb3845

assert(chainPos <= chainSize); /* I believe this is guaranteed... */

cc @felixhandte

[senhuang42]

So I think the two changes should be the correct fix. If I'm not mistaken, the original seems to indicate that there was never any actual adjustment of the DDSS hashLog? Would this change/improve the performance of DDSS?

[felixhandte]

Makes sense.

[felixhandte]

Hm. The original may be mistaken, but I don't think this is a correct fix.

This call is of the form ZSTD_overrideCParams(dst, src). This had been part of a multi-step resolution of params into the local cParams. The intent here is to first get the default cparams for the level, and then override those params with any explicit cparams that were set in the cctxparams by the user. These resolved params are then fed back into the cctxparams (on :4812) and passed down the stack.

With this new version, we will just ignore any explicit cparams set by the user. Which might well fix the assertion failure you're seeing, if the clevel derived params are all safe, but the fuzzer is setting bad cparams that aren't getting caught and rejected.

But then the correct fix would be to correctly validate the cparam set for ddss.

[Cyan4973]

This detailed explanation is probably worth becoming a code comment

[senhuang42]

That makes sense: so the idea here is that if hLog is explicitly set, DDS will not override that by adding 2 to it.

But the actual issue here is that if we set hLog = 7 explicitly, DDS will not override. But later on, ZSTD_dedicatedDictSearch_revertCParams() is called anyways, subtracting 2 from the hLog, making us end up with an invalid hLog of 5 in ZSTD_resetCCtx_byAttachingCDict() here:

zstd/lib/compress/zstd_compress.c

Lines 1996 to 2016 in 3eb3845

	ZSTD_resetCCtx_byAttachingCDict(ZSTD_CCtx* cctx,
	const ZSTD_CDict* cdict,
	ZSTD_CCtx_params params,
	U64 pledgedSrcSize,
	ZSTD_buffered_policy_e zbuff)
	{
	DEBUGLOG(4, "ZSTD_resetCCtx_byAttachingCDict() pledgedSrcSize=%zu", pledgedSrcSize);
	{
	ZSTD_compressionParameters adjusted_cdict_cParams = cdict->matchState.cParams;
	unsigned const windowLog = params.cParams.windowLog;
	assert(windowLog != 0);
	/* Resize working context table params for input only, since the dict
	* has its own tables. */
	/* pledgedSrcSize == 0 means 0! */
	if (cdict->matchState.dedicatedDictSearch) {
	ZSTD_dedicatedDictSearch_revertCParams(&adjusted_cdict_cParams);
	}
	params.cParams = ZSTD_adjustCParams_internal(adjusted_cdict_cParams, pledgedSrcSize,
	cdict->dictContentSize, ZSTD_cpm_attachDict);

At this point in the code, is it even possible to tell whether or not the hLog is an hLog that was explicitly set or an hLog that was adjusted because DDS is available? Ideally, we'd want to be able to detect the latter case, and only subtract from hLog then.

Maybe we can just call ZSTD_clampCParams() on the ZSTD_adjustCParams_internal() call right after, but that would still mean that the hLog could still get reduced when it shouldn't be.

[Cyan4973]

Why would the interpretation of hlog by DDS be different, depending on this parameter being automatically determined or explicitly set ? The way hlog is set should not matter.

Moreover, the interpretation of hlog should remain local.
If DDS considers that a hlog==7 will actually be interpreted as 5 for its own use, that's fine,
but this "interpreted value" should not be confused with the parameter hlog.
As a consequence, this interpretation should remain private within DDS, and in no way impact any other part of the system, not even indirectly.

[senhuang42]

I've pushed @felixhandte's fix for all the assert issues onto this PR in the latest commit, it should fix the issue.

[terrelln]

Thanks @senhuang42, DDS coverage looks good now!