On 11/12/20 4:51 PM, Renat Nurgaliyev wrote: > Missing cast to 32 bits in overflow check makes this conditional never > be true. Because of this, computation of SHA-1 checksum will be wrong > for any data that is bigger than 512 megabytes, which in bits is the > boundary of 32 bits integer. > > In practice it means that any big OVN southbound database, with > transactions bigger than 512 megabytes, is considered corrupt and > ovsdb-server will refuse to work with the database. > > Signed-off-by: Renat Nurgaliyev <[email protected]> > --- > lib/sha1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/sha1.c b/lib/sha1.c > index 4f48ef210..4889c976b 100644 > --- a/lib/sha1.c > +++ b/lib/sha1.c > @@ -202,7 +202,7 @@ sha1_update(struct sha1_ctx *ctx, const void *buffer_, > size_t count) > const uint8_t *buffer = buffer_; > unsigned int i; > > - if ((ctx->count_lo + (count << 3)) < ctx->count_lo) { > + if ((uint32_t)(ctx->count_lo + (count << 3)) < ctx->count_lo) { > ctx->count_hi++; > } > ctx->count_lo += count << 3; >
Good catch! Thanks! Looking at the code and comparing it with original Apache Portable Runtime library it seems that the actual issue is that type of the 'count' changed from 'unsigned int' in the original code to 'size_t' that is 64bit on modern systems. So, I think, instead of the cast we need to change the type of 'count' here and in 'sha1_bytes()' to uint32_t, so we will cover this case and possible other issues related to this. One more thing why we need to change the type is that I'm not sure if functions will work correctly in all cases if someone will call them with chunks of data > UINT32_MAX in size, so it's better to protect ourselves by making it clear that chunk size should fit into uint32_t. What do you think? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
