2020-05-02 06:35:47

by Eric Biggers

[permalink] [raw]
Subject: [PATCH] lib/xxhash: make xxh{32,64}_update() return void

From: Eric Biggers <[email protected]>

The return value of xxh64_update() is pointless and confusing, since an
error is only returned for input==NULL. But the callers must ignore
this error because they might pass input=NULL, length=0.

Likewise for xxh32_update().

Just make these functions return void.

Cc: Nikolay Borisov <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---

lib/xxhash.c doesn't actually have a maintainer, but probably it makes
sense to take this through the crypto tree, alongside the other patch I
sent to return void in lib/crypto/sha256.c.

include/linux/xxhash.h | 8 ++------
lib/xxhash.c | 20 ++++++--------------
2 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/include/linux/xxhash.h b/include/linux/xxhash.h
index 52b073fea17fe4..e1c469802ebdba 100644
--- a/include/linux/xxhash.h
+++ b/include/linux/xxhash.h
@@ -185,10 +185,8 @@ void xxh32_reset(struct xxh32_state *state, uint32_t seed);
* @length: The length of the data to hash.
*
* After calling xxh32_reset() call xxh32_update() as many times as necessary.
- *
- * Return: Zero on success, otherwise an error code.
*/
-int xxh32_update(struct xxh32_state *state, const void *input, size_t length);
+void xxh32_update(struct xxh32_state *state, const void *input, size_t length);

/**
* xxh32_digest() - produce the current xxh32 hash
@@ -218,10 +216,8 @@ void xxh64_reset(struct xxh64_state *state, uint64_t seed);
* @length: The length of the data to hash.
*
* After calling xxh64_reset() call xxh64_update() as many times as necessary.
- *
- * Return: Zero on success, otherwise an error code.
*/
-int xxh64_update(struct xxh64_state *state, const void *input, size_t length);
+void xxh64_update(struct xxh64_state *state, const void *input, size_t length);

/**
* xxh64_digest() - produce the current xxh64 hash
diff --git a/lib/xxhash.c b/lib/xxhash.c
index aa61e2a3802f0a..64bb68a9621ed1 100644
--- a/lib/xxhash.c
+++ b/lib/xxhash.c
@@ -267,21 +267,19 @@ void xxh64_reset(struct xxh64_state *statePtr, const uint64_t seed)
}
EXPORT_SYMBOL(xxh64_reset);

-int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
+void xxh32_update(struct xxh32_state *state, const void *input,
+ const size_t len)
{
const uint8_t *p = (const uint8_t *)input;
const uint8_t *const b_end = p + len;

- if (input == NULL)
- return -EINVAL;
-
state->total_len_32 += (uint32_t)len;
state->large_len |= (len >= 16) | (state->total_len_32 >= 16);

if (state->memsize + len < 16) { /* fill in tmp buffer */
memcpy((uint8_t *)(state->mem32) + state->memsize, input, len);
state->memsize += (uint32_t)len;
- return 0;
+ return;
}

if (state->memsize) { /* some data left from previous update */
@@ -331,8 +329,6 @@ int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
memcpy(state->mem32, p, (size_t)(b_end-p));
state->memsize = (uint32_t)(b_end-p);
}
-
- return 0;
}
EXPORT_SYMBOL(xxh32_update);

@@ -374,20 +370,18 @@ uint32_t xxh32_digest(const struct xxh32_state *state)
}
EXPORT_SYMBOL(xxh32_digest);

-int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
+void xxh64_update(struct xxh64_state *state, const void *input,
+ const size_t len)
{
const uint8_t *p = (const uint8_t *)input;
const uint8_t *const b_end = p + len;

- if (input == NULL)
- return -EINVAL;
-
state->total_len += len;

if (state->memsize + len < 32) { /* fill in tmp buffer */
memcpy(((uint8_t *)state->mem64) + state->memsize, input, len);
state->memsize += (uint32_t)len;
- return 0;
+ return;
}

if (state->memsize) { /* tmp buffer is full */
@@ -436,8 +430,6 @@ int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
memcpy(state->mem64, p, (size_t)(b_end-p));
state->memsize = (uint32_t)(b_end - p);
}
-
- return 0;
}
EXPORT_SYMBOL(xxh64_update);

--
2.26.2


2020-05-02 08:02:17

by Nikolay Borisov

[permalink] [raw]
Subject: Re: [PATCH] lib/xxhash: make xxh{32,64}_update() return void



On 2.05.20 г. 9:34 ч., Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> The return value of xxh64_update() is pointless and confusing, since an
> error is only returned for input==NULL. But the callers must ignore
> this error because they might pass input=NULL, length=0.
>
> Likewise for xxh32_update().
>
> Just make these functions return void.
>
> Cc: Nikolay Borisov <[email protected]>
> Signed-off-by: Eric Biggers <[email protected]>
> ---
>
> lib/xxhash.c doesn't actually have a maintainer, but probably it makes
> sense to take this through the crypto tree, alongside the other patch I
> sent to return void in lib/crypto/sha256.c.
>
> include/linux/xxhash.h | 8 ++------
> lib/xxhash.c | 20 ++++++--------------
> 2 files changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/xxhash.h b/include/linux/xxhash.h
> index 52b073fea17fe4..e1c469802ebdba 100644
> --- a/include/linux/xxhash.h
> +++ b/include/linux/xxhash.h
> @@ -185,10 +185,8 @@ void xxh32_reset(struct xxh32_state *state, uint32_t seed);
> * @length: The length of the data to hash.
> *
> * After calling xxh32_reset() call xxh32_update() as many times as necessary.
> - *
> - * Return: Zero on success, otherwise an error code.
> */
> -int xxh32_update(struct xxh32_state *state, const void *input, size_t length);
> +void xxh32_update(struct xxh32_state *state, const void *input, size_t length);
>
> /**
> * xxh32_digest() - produce the current xxh32 hash
> @@ -218,10 +216,8 @@ void xxh64_reset(struct xxh64_state *state, uint64_t seed);
> * @length: The length of the data to hash.
> *
> * After calling xxh64_reset() call xxh64_update() as many times as necessary.
> - *
> - * Return: Zero on success, otherwise an error code.
> */
> -int xxh64_update(struct xxh64_state *state, const void *input, size_t length);
> +void xxh64_update(struct xxh64_state *state, const void *input, size_t length);
>
> /**
> * xxh64_digest() - produce the current xxh64 hash
> diff --git a/lib/xxhash.c b/lib/xxhash.c
> index aa61e2a3802f0a..64bb68a9621ed1 100644
> --- a/lib/xxhash.c
> +++ b/lib/xxhash.c
> @@ -267,21 +267,19 @@ void xxh64_reset(struct xxh64_state *statePtr, const uint64_t seed)
> }
> EXPORT_SYMBOL(xxh64_reset);
>
> -int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
> +void xxh32_update(struct xxh32_state *state, const void *input,
> + const size_t len)
> {
> const uint8_t *p = (const uint8_t *)input;
> const uint8_t *const b_end = p + len;
>
> - if (input == NULL)
> - return -EINVAL;
> -

Values calculated based on input are dereferenced further down, wouldn't
that cause crashes in case input is null ?

> state->total_len_32 += (uint32_t)len;
> state->large_len |= (len >= 16) | (state->total_len_32 >= 16);
>
> if (state->memsize + len < 16) { /* fill in tmp buffer */
> memcpy((uint8_t *)(state->mem32) + state->memsize, input, len);
> state->memsize += (uint32_t)len;
> - return 0;
> + return;
> }
>
> if (state->memsize) { /* some data left from previous update */
> @@ -331,8 +329,6 @@ int xxh32_update(struct xxh32_state *state, const void *input, const size_t len)
> memcpy(state->mem32, p, (size_t)(b_end-p));
> state->memsize = (uint32_t)(b_end-p);
> }
> -
> - return 0;
> }
> EXPORT_SYMBOL(xxh32_update);
>
> @@ -374,20 +370,18 @@ uint32_t xxh32_digest(const struct xxh32_state *state)
> }
> EXPORT_SYMBOL(xxh32_digest);
>
> -int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
> +void xxh64_update(struct xxh64_state *state, const void *input,
> + const size_t len)
> {
> const uint8_t *p = (const uint8_t *)input;
> const uint8_t *const b_end = p + len;
>
> - if (input == NULL)
> - return -EINVAL;
> -
> state->total_len += len;
>
> if (state->memsize + len < 32) { /* fill in tmp buffer */
> memcpy(((uint8_t *)state->mem64) + state->memsize, input, len);
> state->memsize += (uint32_t)len;
> - return 0;
> + return;
> }
>
> if (state->memsize) { /* tmp buffer is full */
> @@ -436,8 +430,6 @@ int xxh64_update(struct xxh64_state *state, const void *input, const size_t len)
> memcpy(state->mem64, p, (size_t)(b_end-p));
> state->memsize = (uint32_t)(b_end - p);
> }
> -
> - return 0;
> }
> EXPORT_SYMBOL(xxh64_update);
>
>

2020-05-02 16:47:02

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH] lib/xxhash: make xxh{32,64}_update() return void

On Sat, May 02, 2020 at 11:01:35AM +0300, Nikolay Borisov wrote:
> > +void xxh32_update(struct xxh32_state *state, const void *input,
> > + const size_t len)
> > {
> > const uint8_t *p = (const uint8_t *)input;
> > const uint8_t *const b_end = p + len;
> >
> > - if (input == NULL)
> > - return -EINVAL;
> > -
>
> Values calculated based on input are dereferenced further down, wouldn't
> that cause crashes in case input is null ?
>

Only if len > 0, which would mean the caller provided an invalid buffer.

- Eric