| Message ID | 20200502063423.1052614-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | lib/xxhash: make xxh{32,64}_update() return void | expand |
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); > >
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
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
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,
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);