On Mon, Jan 12, 2026 at 3:29 PM Branko Čibej <[email protected]> wrote:
> On 12. 1. 26 14:30, Timofei Zhakov wrote:
>
> 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.
>
>
>
> Is that a fact? Sorry but these days 4gigs is laughably small in certain
> applications. Our API promises "apr_size_t" chunks which on 64-bit
> platforms means larger than 4 GiB. We can change our API or we can fix the
> bug. It doesn't matter if the bug was there before, we know it's there now.
>
> -- Brane
>
>
Okay, it makes sense. I made a patch to support that behaviour. What do you
think?
[[[
Index: subversion/libsvn_subr/checksum_apr.c
===================================================================
--- subversion/libsvn_subr/checksum_apr.c (revision 1931267)
+++ subversion/libsvn_subr/checksum_apr.c (working copy)
@@ -91,12 +91,19 @@
apr_size_t len)
{
apr_sha1_ctx_t sha1_ctx;
+ const char *ptr = data;
- /* Do not blindly truncate the data length. */
- SVN_ERR_ASSERT(len < UINT_MAX);
+ apr_sha1_init(&sha1_ctx);
- apr_sha1_init(&sha1_ctx);
- apr_sha1_update(&sha1_ctx, data, (unsigned int)len);
+ while (len > 0)
+ {
+ unsigned int block = (len < UINT_MAX) ? (unsigned int)len
+ : UINT_MAX;
+ apr_sha1_update(&sha1_ctx, ptr, block);
+ len -= block;
+ ptr += block;
+ }
+
apr_sha1_final(digest, &sha1_ctx);
return SVN_NO_ERROR;
}
@@ -127,9 +134,17 @@
const void *data,
apr_size_t len)
{
- /* Do not blindly truncate the data length. */
- SVN_ERR_ASSERT(len < UINT_MAX);
- apr_sha1_update(&ctx->apr_ctx, data, (unsigned int)len);
+ const char *ptr = data;
+
+ while (len > 0)
+ {
+ unsigned int block = (len < UINT_MAX) ? (unsigned int)len
+ : UINT_MAX;
+ apr_sha1_update(&ctx->apr_ctx, ptr, block);
+ len -= block;
+ ptr += block;
+ }
+
return SVN_NO_ERROR;
}
]]]
I don't really like casting 'void *' to 'char *' but it seems like the only
option.
I'm planning to do a similar thing to the bcrypt backend. A tiny note: long
is usually 32 bit even on 64 bit platforms (on Windows).
--
Timofei Zhakov
Index: subversion/libsvn_subr/checksum_apr.c
===================================================================
--- subversion/libsvn_subr/checksum_apr.c (revision 1931267)
+++ subversion/libsvn_subr/checksum_apr.c (working copy)
@@ -91,12 +91,19 @@
apr_size_t len)
{
apr_sha1_ctx_t sha1_ctx;
+ const char *ptr = data;
- /* Do not blindly truncate the data length. */
- SVN_ERR_ASSERT(len < UINT_MAX);
+ apr_sha1_init(&sha1_ctx);
- apr_sha1_init(&sha1_ctx);
- apr_sha1_update(&sha1_ctx, data, (unsigned int)len);
+ while (len > 0)
+ {
+ unsigned int block = (len < UINT_MAX) ? (unsigned int)len
+ : UINT_MAX;
+ apr_sha1_update(&sha1_ctx, ptr, block);
+ len -= block;
+ ptr += block;
+ }
+
apr_sha1_final(digest, &sha1_ctx);
return SVN_NO_ERROR;
}
@@ -127,9 +134,17 @@
const void *data,
apr_size_t len)
{
- /* Do not blindly truncate the data length. */
- SVN_ERR_ASSERT(len < UINT_MAX);
- apr_sha1_update(&ctx->apr_ctx, data, (unsigned int)len);
+ const char *ptr = data;
+
+ while (len > 0)
+ {
+ unsigned int block = (len < UINT_MAX) ? (unsigned int)len
+ : UINT_MAX;
+ apr_sha1_update(&ctx->apr_ctx, ptr, block);
+ len -= block;
+ ptr += block;
+ }
+
return SVN_NO_ERROR;
}
