Fix a typo that cause inconsistent hash between streaming and stateless way for XXH3 128-bit variant with custom secret and seed 0

[hltj]

I found that the streaming way returns different hash from XXH3_128bits_withSecretandSeed() with seed 0 and a custom secret (not XXH3_kSecret) when the input length not greater than XXH3_MIDSIZE_MAX, while the 64-bit variant works fine.

I read the code and felt that there may be a typo that if there is also if (state->useSeed) in XXH3_128bits_digest(), it will be fine too.
I also added the test cases for that _withSecretandSeed is the same as _withSeed() when len <= XXH3_MIDSIZE_MAX for both 64-bit and 128-bit variant.

[t-mat]

Hi @hltj , thank you for the report and PR.

I also have confirmed your issue and this PR with the following code:
https://gist.github.com/t-mat/700503ca450a4e997e04af84b97fcb7f

• XXH3_128bits_digest() has a bug.
• It only happens when
  • length is less than or equal to XXH3_MIDSIZE_MAX (240).
  • XXH3_state_t::seed is 0.
• This PR # 894 fixes it effectively.

note:

• Output value of xxhsum is not affected by this issue. See cli/xxhsum.c : XSUM_hashStream()

---

@Cyan4973 , @easyaspi314

Although this PR fixes the bug, it is a breaking change, technically.
I mean if this fix will be merged and released in the next release,
XXH3_128bits_digest() may return "different hash value" suddenly.

So, I think we should think how we can/should mitigate it.

• Just fix it.
• Fix it. But also change major version to indicate breaking change / incompatibility in semver way.
  • We're still in 0.x though.
• Don't fix it directly, but set [[deprecated]]. And add new API (or API set) which works properly.
• Add compile time option which fixes this bug.
  • We should decide the default value/behavior of it.

[hltj]

Hi @t-mat , thank you so much for confirming, explaining the details as well as the fix/release suggestions.

The only I want to clarify that it is not XXH3_128bits_withSecretandSeed()'s issue, but XXH3_128bits_digest's. And the change will not break the behavior of XXH3_128bits_withSecretandSeed(), but it will break the behavior of XXH3_128bits_digest() in the corner case.

[t-mat]

@hltj thanks! You're right. I've fixed my comment to prevent further misunderstanding.

[easyaspi314]

• Just fix it.
• Fix it. But also change major version to indicate breaking change / incompatibility in semver way.
• We're still in 0.x though.
• Don't fix it directly, but set [[deprecated]]. And add new API (or API set) which works properly.
• Add compile time option which fixes this bug.
• We should decide the default value/behavior of it.

Idea: Since secretAndSeed is the only thing broken afaik, we only need one API to wrap.

1. Make a new API for the secretAndSeed reset
2. Macro define it to a new name:

// do the namespace and stuff
XXH_DEPRECATED void XXH3_reset_withSecretandSeed();
void XXH3_reset_withSecretandSeed_new();
// end of code
#define XXH3_reset_withSecretandSeed_old XXH3_reset_withSecretandSeed
#ifndef XXH3_OLD_RESET_WITH_SECRET_AND_SEED
#  define XXH3_reset_withSecretandSeed XXH3_reset_withSecretandSeed_new
#endif

We have used opt-in macros when we did the internal API rename.
3. Repurpose useSeed as stateType where we can set a tag for the old API:

enum XXH3_stateType {
    XXH3_STATE_TYPE_SECRET,
    XXH3_STATE_TYPE_SEED,
    XXH3_STATE_TYPE_OLD_SECRET_AND_SEED
};

Then in XXH3_128bits_finalize, do

if (state->stateType == XXH3_STATE_TYPE_SEED
    || XXH_unlikely(state->stateType == XXH3_STATE_TYPE_OLD_SECRET_AND_SEED && state->seed != 0)
)

And in XXH3_64bits_finalize we can do state->stateType != XXH3_STATE_TYPE_SECRET.

Make sure we make this very clear in documentation and comments.

This way, apps compiled against libxxhash.so won't change since they will use the old symbol, and defining that macro would make theoretical "old apps" that expect the broken hash values for some reason work.

[Cyan4973]

Coming back on this topic,
after reading the proposed code change, and the comments,

it looks to me that this is a pretty clear case of detecting and fixing a bug.

XXH3_128bits_withSecretandSeed() has a very clear contract, which is btw the same as XXH3_64bits_withSecretandSeed():

These variants generate hash values using either:

• seed for "short" keys (< XXH3_MIDSIZE_MAX = 240 bytes)
• secret for "large" keys (>= XXH3_MIDSIZE_MAX).

So deviating from this contract is a bug, clear and simple.

Beyond fixing the bug, what matters is to build an ability to detect such bug in the future, which this PR also provides.

So, as far as this PR is concerned, I think it's good to go.

The next question is probably: does this fix deserves a new release ? or even an urgent release ?

Well, I believe that the entry point XXH3_128bits_withSecretandSeed(), which is the only one impacted, is not employed much out there. As far as I can tell, it's fairly niche, otherwise we would have received more bug reports of this kind.
And it is likely going to remain so in the foreseeable future, since the primary motivation for this prototype was to reduce latency in the case where a seed is employed and input size is >= 240 bytes forcing the creation of a custom secret at every hash (which only matters if input is fairly small, like ~1 KB range). That's a very specific scenario.
Furthermore, since the bug is only active when seed = 0, and this case essentially defeats the point of using XXH3_128bits_withSecretandSeed() (just use regular XXH3_128bits() in this case).
edit : and now that I'm analyzing the tests, I realize there are even more conditions:

• the secret must be different from the one generated with XXH3_generateSecret_fromSeed()
• the bug is only active when using Streaming mode

Given this pretty huge list of pre-conditions, I don't anticipate any real-world impact.

That being said, this bug definitely deserves a mention on the release page.

[Cyan4973]

As a side note:

Beyond fixing the bug, what matters is to build an ability to detect such bug in the future, which this PR also provides.

I was testing this capability,
and, sure it works fine for sanity_test,
but it does not for xxhsum's internal sanity test (which is quicker / less extensive).

That's because, even though these tools use almost the same code,
it's not the same code, meaning similar functions are just duplicated.

We may benefit from having a single source of truth for the common parts of the code base,
so that updates like this one benefit both targets.

[Cyan4973]

Thanks @hltj , that's a great investigation and great fix !