Skip to content

hash: Implement xxHash #6524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

hash: Implement xxHash #6524

wants to merge 1 commit into from

Conversation

weltling
Copy link
Contributor

The implementation bundles the xxHash v0.8.0 release and includes all the variants

  • xxh32, 32-bit wide
  • xxh64, 64-bit wide
  • xxh3, 64-bit wide
  • xxh128, 128-bit wide

An initial hash state can be passed through the options arrray. An additional
functionality not targeted in this implementation is the secret support in xxh3
and xxh128. That can be added at a later point.

The serialization for xxh3 and xxh128 should not be implemented, as the
state would contain the secret. Despite the xxHash is a non crypto
algorithm, the secret would be serialized as plain text which would be
insecure.

Signed-off-by: Anatol Belski ab@php.net

The implementation bundles the xxHash v0.8.0 release and includes all the variants

- xxh32, 32-bit wide
- xxh64, 64-bit wide
- xxh3, 64-bit wide
- xxh128, 128-bit wide

An initial hash state can be passed through the options arrray. An additional
functionality not targeted in this implementation is the secret support in xxh3
and xxh128. That can be added at a later point.

The serialization for xxh3 and xxh128 should not be implemented, as the
state would contain the secret. Despite the xxHash is a non crypto
algorithm, the secret would be serialized as plain text which would be
insecure.

Signed-off-by: Anatol Belski <ab@php.net>
@weltling
Copy link
Contributor Author

@m6w6 @nikic ^

@weltling
Copy link
Contributor Author

weltling commented Jan 5, 2021

Would someone have time to take a look?

Thanks.

Copy link
Contributor

@m6w6 m6w6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@weltling
Copy link
Contributor Author

weltling commented Jan 8, 2021

Thanks for the check! I'll plan to finish this up soon.

@php-pulls php-pulls closed this in 23590f7 Jan 9, 2021
@nikic
Copy link
Member

nikic commented Jan 11, 2021

The serialization for xxh3 and xxh128 should not be implemented, as the
state would contain the secret. Despite the xxHash is a non crypto
algorithm, the secret would be serialized as plain text which would be
insecure.

I don't get your reasoning here. In what way is xxHash different from all other hash algorithms? In the paragraph before you also say that secret support is not targeted.

@weltling
Copy link
Contributor Author

Hi, thanks for checking. To compare is here XXH3_state_s vs. for example XXH64_state_s. Both XXH3 and XXH128 will use the former and they support passing a secret that will be involved, to check XXH3_update for an example. Adding the support for it is as easy as extending the config options to support it. I'd eventually do it, so knowing that ahead it was clear these two should not be serialized. The call would then look similar to $h = hash("xx3", $str, options: ["secret" => "my secret"]);.

Another part here - as one can see, the secret in XXH3_state_s is a pointer. That means, it can't be stored by just using a serialization format, it needs to be handled a specific way. We previously had a discussion on an approach to serialize the initial seed state in the murmur PR. For the time being it's not serialized as nothing requires the initial seed state after the init (though it would be useful for the error reporting,). Serializing a "snapshot" of the context consisting on just scalar datatypes seems fine enough and doesn't overload the context struct itself. I guess this part brings us back to that discussion and seems cases like this would still require saving data into the php object part, not into the actual context struct. Still in xxHash case there isn't a hard requirement to serialize the secret as it would be insecure. I guess the more cases come, some strategy might show more clear.

Thanks

jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Mar 9, 2022
> **MurmurHash3**
>
> Added support for MurmurHash3 with streaming support. The following variants are implemented:
> * murmur3a, 32-bit hash
> * murmur3c, 128-bit hash for x86
> * murmur3f, 128-bit hash for x64
>
> **xxHash**
>
> Added support for xxHash. The following variants are implemented:
> * xxh32, 32-bit hash
> * xxh64, 64-bit hash
> * xxh3, 64-bit hash
> * xxh128, 128-bit hash

Includes unit tests.

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.murmurhash3
* php/php-src#6059
* php/php-src@72e91e9
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.xxhash
* php/php-src#6524
* php/php-src@23590f7
jrfnl added a commit to jrfnl/doc-en that referenced this pull request Mar 11, 2022
> **MurmurHash3**
>
> Added support for MurmurHash3 with streaming support. The following variants are implemented:
> * murmur3a, 32-bit hash
> * murmur3c, 128-bit hash for x86
> * murmur3f, 128-bit hash for x64
>
> **xxHash**
>
> Added support for xxHash. The following variants are implemented:
> * xxh32, 32-bit hash
> * xxh64, 64-bit hash
> * xxh3, 64-bit hash
> * xxh128, 128-bit hash

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.murmurhash3
* php/php-src#6059
* php/php-src@72e91e9
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.xxhash
* php/php-src#6524
* php/php-src@23590f7
jrfnl added a commit to jrfnl/doc-en that referenced this pull request Mar 11, 2022
> **MurmurHash3**
>
> Added support for MurmurHash3 with streaming support. The following variants are implemented:
> * murmur3a, 32-bit hash
> * murmur3c, 128-bit hash for x86
> * murmur3f, 128-bit hash for x64
>
> **xxHash**
>
> Added support for xxHash. The following variants are implemented:
> * xxh32, 32-bit hash
> * xxh64, 64-bit hash
> * xxh3, 64-bit hash
> * xxh128, 128-bit hash

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.murmurhash3
* php/php-src#6059
* php/php-src@72e91e9
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.xxhash
* php/php-src#6524
* php/php-src@23590f7
jrfnl added a commit to jrfnl/doc-en that referenced this pull request Mar 11, 2022
> **MurmurHash3**
>
> Added support for MurmurHash3 with streaming support. The following variants are implemented:
> * murmur3a, 32-bit hash
> * murmur3c, 128-bit hash for x86
> * murmur3f, 128-bit hash for x64
>
> **xxHash**
>
> Added support for xxHash. The following variants are implemented:
> * xxh32, 32-bit hash
> * xxh64, 64-bit hash
> * xxh3, 64-bit hash
> * xxh128, 128-bit hash

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.murmurhash3
* php/php-src#6059
* php/php-src@72e91e9
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.xxhash
* php/php-src#6524
* php/php-src@23590f7
cmb69 pushed a commit to php/doc-en that referenced this pull request Mar 11, 2022
> **MurmurHash3**
>
> Added support for MurmurHash3 with streaming support. The following variants are implemented:
> * murmur3a, 32-bit hash
> * murmur3c, 128-bit hash for x86
> * murmur3f, 128-bit hash for x64
>
> **xxHash**
>
> Added support for xxHash. The following variants are implemented:
> * xxh32, 32-bit hash
> * xxh64, 64-bit hash
> * xxh3, 64-bit hash
> * xxh128, 128-bit hash

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.murmurhash3
* php/php-src#6059
* php/php-src@72e91e9
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.xxhash
* php/php-src#6524
* php/php-src@23590f7

Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>

Closes GH-1451.
tiffany-taylor pushed a commit to tiffany-taylor/doc-en that referenced this pull request Jan 16, 2023
> **MurmurHash3**
>
> Added support for MurmurHash3 with streaming support. The following variants are implemented:
> * murmur3a, 32-bit hash
> * murmur3c, 128-bit hash for x86
> * murmur3f, 128-bit hash for x64
>
> **xxHash**
>
> Added support for xxHash. The following variants are implemented:
> * xxh32, 32-bit hash
> * xxh64, 64-bit hash
> * xxh3, 64-bit hash
> * xxh128, 128-bit hash

Refs:
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.murmurhash3
* php/php-src#6059
* php/php-src@72e91e9
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.hash.xxhash
* php/php-src#6524
* php/php-src@23590f7

Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>

Closes phpGH-1451.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants