* [PATCH] lib/xxhash: make xxh{32,64}_update() return void
@ 2020-05-02 6:34 Eric Biggers
2020-05-02 8:01 ` Nikolay Borisov
2020-05-15 19:15 ` Eric Biggers
0 siblings, 2 replies; 5+ messages in thread
From: Eric Biggers @ 2020-05-02 6:34 UTC (permalink / raw)
To: linux-crypto; +Cc: Nikolay Borisov
From: Eric Biggers <ebiggers@google.com>
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 <nborisov@suse.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] lib/xxhash: make xxh{32,64}_update() return void
2020-05-02 6:34 [PATCH] lib/xxhash: make xxh{32,64}_update() return void Eric Biggers
@ 2020-05-02 8:01 ` Nikolay Borisov
2020-05-02 16:46 ` Eric Biggers
2020-05-15 19:15 ` Eric Biggers
1 sibling, 1 reply; 5+ messages in thread
From: Nikolay Borisov @ 2020-05-02 8:01 UTC (permalink / raw)
To: Eric Biggers, linux-crypto
On 2.05.20 г. 9:34 ч., Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> 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 <nborisov@suse.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>
> 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);
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] lib/xxhash: make xxh{32,64}_update() return void
2020-05-02 8:01 ` Nikolay Borisov
@ 2020-05-02 16:46 ` Eric Biggers
0 siblings, 0 replies; 5+ messages in thread
From: Eric Biggers @ 2020-05-02 16:46 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-crypto
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/xxhash: make xxh{32,64}_update() return void
2020-05-02 6:34 [PATCH] lib/xxhash: make xxh{32,64}_update() return void Eric Biggers
2020-05-02 8:01 ` Nikolay Borisov
@ 2020-05-15 19:15 ` Eric Biggers
2020-05-19 4:38 ` Herbert Xu
1 sibling, 1 reply; 5+ messages in thread
From: Eric Biggers @ 2020-05-15 19:15 UTC (permalink / raw)
To: linux-crypto, Herbert Xu; +Cc: Nikolay Borisov
On Fri, May 01, 2020 at 11:34:23PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> 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 <nborisov@suse.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>
> 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.
Herbert, are you planning to apply this patch?
- Eric
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] lib/xxhash: make xxh{32,64}_update() return void
2020-05-15 19:15 ` Eric Biggers
@ 2020-05-19 4:38 ` Herbert Xu
0 siblings, 0 replies; 5+ messages in thread
From: Herbert Xu @ 2020-05-19 4:38 UTC (permalink / raw)
To: Eric Biggers, Nick Terrell, Chris Mason; +Cc: linux-crypto, Nikolay Borisov
On Fri, May 15, 2020 at 12:15:16PM -0700, Eric Biggers wrote:
> On Fri, May 01, 2020 at 11:34:23PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > 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 <nborisov@suse.com>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >
> > 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.
>
> Herbert, are you planning to apply this patch?
It looks like Chris Mason added this originally. Chris, are you
OK with this patch and if so do you want to take it or shall I
push it through the crypto tree?
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-05-19 4:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-02 6:34 [PATCH] lib/xxhash: make xxh{32,64}_update() return void Eric Biggers
2020-05-02 8:01 ` Nikolay Borisov
2020-05-02 16:46 ` Eric Biggers
2020-05-15 19:15 ` Eric Biggers
2020-05-19 4:38 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).