On Mon, Jan 12, 2026 at 1:22 PM Branko Čibej <[email protected]> wrote:
> On 12. 1. 26 12:52, [email protected] wrote: > > Author: brane > Date: Mon Jan 12 11:52:07 2026 > New Revision: 1931262 > > Log: > Fix warnings in the new checksum code. > > * subversion/libsvn_subr/checksum.c > (svn_checksum): Remove unused variable. > * subversion/libsvn_subr/checksum_apr.c: Include <limits.h>. > (svn_checksum__sha1, svn_checksum__sha1_ctx_update): Do not blindly cast > or implicitly narrow the data length. Check the limits first. > > Modified: > subversion/trunk/subversion/libsvn_subr/checksum.c > subversion/trunk/subversion/libsvn_subr/checksum_apr.c > > Modified: subversion/trunk/subversion/libsvn_subr/checksum.c > ============================================================================== > --- subversion/trunk/subversion/libsvn_subr/checksum.c Mon Jan 12 > 11:51:21 2026 (r1931261) > +++ subversion/trunk/subversion/libsvn_subr/checksum.c Mon Jan 12 > 11:52:07 2026 (r1931262) > @@ -475,8 +475,6 @@ svn_checksum(svn_checksum_t **checksum, > apr_size_t len, > apr_pool_t *pool) > { > - apr_sha1_ctx_t sha1_ctx; > - > SVN_ERR(validate_kind(kind)); > *checksum = svn_checksum_create(kind, pool); > > > Modified: subversion/trunk/subversion/libsvn_subr/checksum_apr.c > ============================================================================== > --- subversion/trunk/subversion/libsvn_subr/checksum_apr.c Mon Jan 12 > 11:51:21 2026 (r1931261) > +++ subversion/trunk/subversion/libsvn_subr/checksum_apr.c Mon Jan 12 > 11:52:07 2026 (r1931262) > @@ -21,6 +21,8 @@ > * ==================================================================== > */ > > +#include <limits.h> > + > #include "svn_private_config.h" > #ifdef SVN_CHECKSUM_BACKEND_APR > > @@ -89,6 +91,10 @@ svn_checksum__sha1(unsigned char *digest > apr_size_t len) > { > apr_sha1_ctx_t sha1_ctx; > + > + /* Do not blindly truncate the data length. */ > + SVN_ERR_ASSERT(len < UINT_MAX); > + > apr_sha1_init(&sha1_ctx); > apr_sha1_update(&sha1_ctx, data, (unsigned int)len); > apr_sha1_final(digest, &sha1_ctx); > @@ -121,7 +127,9 @@ svn_checksum__sha1_ctx_update(svn_checks > const void *data, > apr_size_t len) > { > - apr_sha1_update(&ctx->apr_ctx, data, len); > + /* Do not blindly truncate the data length. */ > + SVN_ERR_ASSERT(len < UINT_MAX); > + apr_sha1_update(&ctx->apr_ctx, data, (unsigned int)len); > return SVN_NO_ERROR; > } > > > Just a quick note, it's generally not a good idea to use unchecked > typecasts anywhere. In this case there was a borderline chance of > Subversion quietly computing invalid hashes. The assert at least removes > the "quiet" part, although I suspect the better solution is to call > apr_sha1_update in UINT_MAX-sized chunks until all data are consumed. > I agree. It would be great if APR was accepting size as size_t. But it's not a big deal. Anyways, I'm pretty sure the issue existed there before. Maybe this function could feed it blocks of UINT_MAX if it exceeds the limit. But I don't think we should bother to support such edge cases, because if it happens, the issue is on the caller side -- it should not allocate 4GB buffers in memory. > There is potentially a similar problem in checksum_bcrypt.c, where there > are typecasts to ULONG. Now, currently apr_size_t and ULONG are the same on > both 32- and 64-bit Windows ... until they aren't, and no-one is going to > see the problem because the Windows build is plagued with warnings and new > ones will just get lost in the noise. > > -- Brane > > -- Timofei Zhakov
