Repository navigation
[Outreachy PATCH] diff-delta: use safe integer arithmetic - #2276
Open
poojarysrujith wants to merge 1 commit into
Open
poojarysrujith wants to merge 1 commit into
poojarysrujith wants to merge 1 commit into
Conversation
poojarysrujith
force-pushed
the
c-refactor-target-name
branch
2 times, most recently
from
October 22, 2026 17:08
654c84c to
a0aafe6
Compare
Author
|
/preview |
|
Preview email sent as pull.2276.git.1792727673390.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2276.git.1792727808109.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
poojarysrujith
force-pushed
the
c-refactor-target-name
branch
2 times, most recently
from
October 23, 2026 05:38
9f26682 to
14ffd63
Compare
Author
|
/preview |
|
Preview email sent as pull.2276.v2.git.1792734840488.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2276.v2.git.1792734993812.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Srujith A Poojary <poojarysrujith@gmail.com>
>
> Replace manual multiplication and addition in create_delta_index()
> with safe integer helpers (st_mult, st_add, st_add3) to prevent
> overflows while preserving NULL returns on failure.
I do not quite understand what the last part of this explanation
wants to say, i.e., "while preserving NULL returns on failure". It
makes it sound as if the function would return NULL if the integer
helpers detect integer overflow, but I do not think that is what
actually the updated code does at all. st_add() and friends will
simply die() when overflow happens, so you will not even reach
malloc(), let alone return NULL when the malloc fails.
And if you reply to this review message, please do not top-post.
Thanks.
>
> Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> ---
> [Outreachy PATCH] diff-delta: use safe integer arithmetic
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2276%2Fpoojarysrujith%2Fc-refactor-target-name-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2276/poojarysrujith/c-refactor-target-name-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2276
>
> diff-delta.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/diff-delta.c b/diff-delta.c
> index 7cbedeb507..45df070ab9 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -162,8 +162,8 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> hmask = hsize - 1;
>
> /* allocate lookup index */
> - memsize = sizeof(*hash) * hsize +
> - sizeof(*entry) * entries;
> + memsize = st_add(st_mult(sizeof(*hash), hsize),
> + st_mult(sizeof(*entry), entries));
> mem = malloc(memsize);
> if (!mem)
> return NULL;
> @@ -258,9 +258,9 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> * Now create the packed index in array form
> * rather than linked lists.
> */
> - memsize = sizeof(*index)
> - + sizeof(*packed_hash) * (hsize+1)
> - + sizeof(*packed_entry) * entries;
> + memsize = st_add3(sizeof(*index),
> + st_mult(sizeof(*packed_hash), hsize + 1),
> + st_mult(sizeof(*packed_entry), entries));
> mem = malloc(memsize);
> if (!mem) {
> free(hash);
>
> base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926 |
|
Srujith Poojary wrote on the Git mailing list (how to reply to this email): On Fri, Oct 23, 2026 at 10:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Srujith A Poojary <poojarysrujith@gmail.com>
> >
> > Replace manual multiplication and addition in create_delta_index()
> > with safe integer helpers (st_mult, st_add, st_add3) to prevent
> > overflows while preserving NULL returns on failure.
>
> I do not quite understand what the last part of this explanation
> wants to say, i.e., "while preserving NULL returns on failure". It
> makes it sound as if the function would return NULL if the integer
> helpers detect integer overflow, but I do not think that is what
> actually the updated code does at all. st_add() and friends will
> simply die() when overflow happens, so you will not even reach
> malloc(), let alone return NULL when the malloc fails.
>
> And if you reply to this review message, please do not top-post.
>
> Thanks.
>
> >
> > Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> > ---
> > [Outreachy PATCH] diff-delta: use safe integer arithmetic
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2276%2Fpoojarysrujith%2Fc-refactor-target-name-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2276/poojarysrujith/c-refactor-target-name-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/2276
> >
> > diff-delta.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/diff-delta.c b/diff-delta.c
> > index 7cbedeb507..45df070ab9 100644
> > --- a/diff-delta.c
> > +++ b/diff-delta.c
> > @@ -162,8 +162,8 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> > hmask = hsize - 1;
> >
> > /* allocate lookup index */
> > - memsize = sizeof(*hash) * hsize +
> > - sizeof(*entry) * entries;
> > + memsize = st_add(st_mult(sizeof(*hash), hsize),
> > + st_mult(sizeof(*entry), entries));
> > mem = malloc(memsize);
> > if (!mem)
> > return NULL;
> > @@ -258,9 +258,9 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> > * Now create the packed index in array form
> > * rather than linked lists.
> > */
> > - memsize = sizeof(*index)
> > - + sizeof(*packed_hash) * (hsize+1)
> > - + sizeof(*packed_entry) * entries;
> > + memsize = st_add3(sizeof(*index),
> > + st_mult(sizeof(*packed_hash), hsize + 1),
> > + st_mult(sizeof(*packed_entry), entries));
> > mem = malloc(memsize);
> > if (!mem) {
> > free(hash);
> >
> > base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
Hi Junio,
Thank you for clarifying this. You are right—st_add() and st_mult()
call die() on overflow rather than returning NULL.
I will update the commit message to accurately reflect this behavior
and submit v2.
Best regards,
Srujith |
poojarysrujith
force-pushed
the
c-refactor-target-name
branch
from
October 23, 2026 16:43
14ffd63 to
ae502df
Compare
|
Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Srujith
On 23/10/2026 06:56, Srujith A Poojary via GitGitGadget wrote:
> From: Srujith A Poojary <poojarysrujith@gmail.com>
> > Replace manual multiplication and addition in create_delta_index()
> with safe integer helpers (st_mult, st_add, st_add3) to prevent
> overflows.
Sounds like a good idea
> diff --git a/diff-delta.c b/diff-delta.c
> index 7cbedeb507..45df070ab9 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -162,8 +162,8 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> hmask = hsize - 1;
> > /* allocate lookup index */
> - memsize = sizeof(*hash) * hsize +
> - sizeof(*entry) * entries;
> + memsize = st_add(st_mult(sizeof(*hash), hsize),
> + st_mult(sizeof(*entry), entries));
The alignment is off here, see https://github.com/gitgitgadget/git/actions/runs/39906906057/job/120260845179?pr=2276 for a suggested fix from clang-format. I think you should be able to use "git clang-format HEAD^" to fix it.
> mem = malloc(memsize);
> if (!mem)
> return NULL;
> @@ -258,9 +258,9 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> * Now create the packed index in array form
> * rather than linked lists.
> */
> - memsize = sizeof(*index)
> - + sizeof(*packed_hash) * (hsize+1)
> - + sizeof(*packed_entry) * entries;
> + memsize = st_add3(sizeof(*index),
> + st_mult(sizeof(*packed_hash), hsize + 1),
What are the bounds of hsize? Can adding one to it overflow? The alignment is off here as well, all the arguments to st_add3() should be aligned.
Thanks
Phillip
> + st_mult(sizeof(*packed_entry), entries));
> mem = malloc(memsize);
> if (!mem) {
> free(hash);
> > base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926 |
|
User |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Srujith Poojary <poojarysrujith@gmail.com> writes:
> Thank you for clarifying this. You are right—st_add() and st_mult()
> call die() on overflow rather than returning NULL.
> I will update the commit message to accurately reflect this behavior
> and submit v2.
Please don't, before making sure if it can overflow. With the
number of entries capped like this:
if (bufsize >= 0xffffffffUL)
entries = 0xfffffffeU / RABIN_WINDOW;
and then computed
hsize = entries / 4;
for (i = 4; (1u << i) < hsize; i++);
hsize = 1 << i;
/* allocate lookup index */
memsize = sizeof(*hash) * hsize +
sizeof(*entry) * entries;
it may not be possible to overflow size_t even on 32-bit platforms.
I didn't have time to do math yet myself, though.
|
|
Srujith Poojary wrote on the Git mailing list (how to reply to this email): On Fri, Oct 23, 2026 at 9:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Srujith Poojary <poojarysrujith@gmail.com> writes:
>
> > Thank you for clarifying this. You are right—st_add() and st_mult()
> > call die() on overflow rather than returning NULL.
> > I will update the commit message to accurately reflect this behavior
> > and submit v2.
>
> Please don't, before making sure if it can overflow. With the
> number of entries capped like this:
>
> if (bufsize >= 0xffffffffUL)
> entries = 0xfffffffeU / RABIN_WINDOW;
>
> and then computed
>
> hsize = entries / 4;
> for (i = 4; (1u << i) < hsize; i++);
> hsize = 1 << i;
> /* allocate lookup index */
> memsize = sizeof(*hash) * hsize +
> sizeof(*entry) * entries;
>
> it may not be possible to overflow size_t even on 32-bit platforms.
> I didn't have time to do math yet myself, though.
>
Hi Phillip and Junio,
Thank you for the detailed feedback.
I checked the bounds: since entries is capped by (0xffffffu /
RABIN_WINDOW), hsize remains small, making a size_t overflow
impossible even on 32-bit systems.
I ran git clang-format HEAD^ to fix the code alignment as suggested
and updated the patch.
Best regards,
Srujith |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Srujith Poojary <poojarysrujith@gmail.com> writes:
> On Fri, Oct 23, 2026 at 9:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Srujith Poojary <poojarysrujith@gmail.com> writes:
>>
>> > Thank you for clarifying this. You are right—st_add() and st_mult()
>> > call die() on overflow rather than returning NULL.
>> > I will update the commit message to accurately reflect this behavior
>> > and submit v2.
>>
>> Please don't, before making sure if it can overflow. With the
>> number of entries capped like this:
>>
>> if (bufsize >= 0xffffffffUL)
>> entries = 0xfffffffeU / RABIN_WINDOW;
>>
>> and then computed
>>
>> hsize = entries / 4;
>> for (i = 4; (1u << i) < hsize; i++);
>> hsize = 1 << i;
>> /* allocate lookup index */
>> memsize = sizeof(*hash) * hsize +
>> sizeof(*entry) * entries;
>>
>> it may not be possible to overflow size_t even on 32-bit platforms.
>> I didn't have time to do math yet myself, though.
>>
>
> Hi Phillip and Junio,
>
> Thank you for the detailed feedback.
>
> I checked the bounds: since entries is capped by (0xffffffu /
> RABIN_WINDOW), hsize remains small, making a size_t overflow
> impossible even on 32-bit systems.
Hmph, I actually came to a different conclusion, but my math is
probably weaker X-<.
entries is capped by 1^32 / 16 = 1^28, and hsize is capped at 1^26.
sizeof(*hash) is 4 bytes on 32-bit platforms.
struct index_entry has a pointer and an unsigned, i.e. 8 bytes.
struct unpacked_index_entry has the above index_entry plus a
pointer, i.e. 8+4=12 bytes or 16 bytes (with 4-byte padding).
That is the sizeof(*entry).
If "struct unpacked_index_entry" has 4-byte padding to align at
8-byte boundary, then memsize = 4 * 1^26 + 16 * 1^28 can overflow
size_t's range that is 1^32. If the struct does not have extra
padding, this would fit below 1^32. We have no guarantee what the
compiler would do, so we need to assume the worst.
Of course it is entirely a different matter if malloc() would allow
such a large allocation even if math does not overflow, but the
focus of this patch is to make sure memsize is computed as a large
allocation in such a case without wrapping around.
|
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Phillip Wood <phillip.wood123@gmail.com> writes:
>> + memsize = st_add3(sizeof(*index),
>> + st_mult(sizeof(*packed_hash), hsize + 1),
>
> What are the bounds of hsize? Can adding one to it overflow? The
> alignment is off here as well, all the arguments to st_add3() should be
> aligned.
Very good question, and I think hsize is capped at 1^26 in a code
early in this function, which makes this safe. It needs to be
explained why it is safe in the log message, though. |
poojarysrujith
force-pushed
the
c-refactor-target-name
branch
from
October 24, 2026 03:35
ae502df to
d288bfe
Compare
Author
|
/submit |
|
Submitted as pull.2276.v3.git.1792813697888.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Srujith Poojary wrote on the Git mailing list (how to reply to this email): On Sat, Oct 24, 2026 at 12:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> + memsize = st_add3(sizeof(*index),
> >> + st_mult(sizeof(*packed_hash), hsize + 1),
> >
> > What are the bounds of hsize? Can adding one to it overflow? The
> > alignment is off here as well, all the arguments to st_add3() should be
> > aligned.
>
> Very good question, and I think hsize is capped at 1^26 in a code
> early in this function, which makes this safe. It needs to be
> explained why it is safe in the log message, though.
Hi Junio,
Thank you for analyzing the struct alignment and memory allocation limits.
I have updated the commit message to explain why st_mult/st_add are
necessary on 32-bit platforms due to padding, as well as documenting
that hsize + 1 is safe because hsize is capped at 2^26.
I also fixed the code formatting using git clang-format HEAD^.
Best regards,
Srujith |
Replace manual multiplication and addition in create_delta_index() with safe integer helpers (st_mult, st_add, st_add3) to prevent potential overflows when computing memory allocation sizes. For the unpacked index, we allocate hsize instances of hash pointers and entries instances of struct index_entry: - hsize is capped at 2^26. - entries is capped at 2^28. On 32-bit platforms with 4-byte padding for struct unpacked_index_entry, the calculation size (sizeof(*hash) * hsize + sizeof(*entry) * entries) can exceed 2^32, making safe integer arithmetic necessary. Similarly, for the packed index, we compute allocation size across struct index, packed_hash, and entries. Using safe helpers prevents wrap-around across both allocations. Note that passing (hsize + 1) as an argument to st_add3() is safe from overflow because hsize is capped at 2^26 early in the function. Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
poojarysrujith
force-pushed
the
c-refactor-target-name
branch
from
October 24, 2026 17:22
d288bfe to
0d5a729
Compare
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Srujith A Poojary <poojarysrujith@gmail.com>
>
> Replace manual multiplication and addition in create_delta_index()
> with safe integer helpers (st_mult, st_add, st_add3) to prevent potential
> overflows when computing memory allocation sizes.
>
> On 32-bit platforms, struct alignment padding could cause the total
> allocation size calculation to exceed 2^32, making safe integer arithmetic
> necessary.
I wonder if "X could cause Y" above skips a bit too many steps. I
was expecting more along the lines of ...
We will need to allocate N instances of A and M instances of B, N
and M are capped to X and Y respectively by this code
... code snippet ...
and size of an instance of A is 8 and size of B could be either W or
Z. When size of B is W, then the total allocation would be ...
And that analysis applies only on the first allocation. I didn't
even look at the packed side of the allocation size, expecting that
you would know how to analyze after seeing my example on the other
allocation.
> Note that hsize + 1 inside st_add3() is safe from overflow
> because hsize is capped at 2^26 early in the function.
It is misleading to say hsize+1 is inside st_add3(), though.
> diff --git a/diff-delta.c b/diff-delta.c
> index 7cbedeb507..efebce3aca 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -162,8 +162,8 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> hmask = hsize - 1;
>
> /* allocate lookup index */
> - memsize = sizeof(*hash) * hsize +
> - sizeof(*entry) * entries;
> + memsize = st_add(st_mult(sizeof(*hash), hsize),
> + st_mult(sizeof(*entry), entries));
> mem = malloc(memsize);
> if (!mem)
> return NULL;
> @@ -258,9 +258,9 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> * Now create the packed index in array form
> * rather than linked lists.
> */
> - memsize = sizeof(*index)
> - + sizeof(*packed_hash) * (hsize+1)
> - + sizeof(*packed_entry) * entries;
> + memsize = st_add3(sizeof(*index),
> + st_mult(sizeof(*packed_hash), hsize + 1),
> + st_mult(sizeof(*packed_entry), entries));
> mem = malloc(memsize);
> if (!mem) {
> free(hash);
>
> base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926 |
|
Srujith Poojary wrote on the Git mailing list (how to reply to this email): On Sat, Oct 24, 2026 at 9:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Srujith A Poojary <poojarysrujith@gmail.com>
> >
> > Replace manual multiplication and addition in create_delta_index()
> > with safe integer helpers (st_mult, st_add, st_add3) to prevent potential
> > overflows when computing memory allocation sizes.
> >
> > On 32-bit platforms, struct alignment padding could cause the total
> > allocation size calculation to exceed 2^32, making safe integer arithmetic
> > necessary.
>
> I wonder if "X could cause Y" above skips a bit too many steps. I
> was expecting more along the lines of ...
>
> We will need to allocate N instances of A and M instances of B, N
> and M are capped to X and Y respectively by this code
>
> ... code snippet ...
>
> and size of an instance of A is 8 and size of B could be either W or
> Z. When size of B is W, then the total allocation would be ...
>
> And that analysis applies only on the first allocation. I didn't
> even look at the packed side of the allocation size, expecting that
> you would know how to analyze after seeing my example on the other
> allocation.
>
> > Note that hsize + 1 inside st_add3() is safe from overflow
> > because hsize is capped at 2^26 early in the function.
>
> It is misleading to say hsize+1 is inside st_add3(), though.
>
>
> > diff --git a/diff-delta.c b/diff-delta.c
> > index 7cbedeb507..efebce3aca 100644
> > --- a/diff-delta.c
> > +++ b/diff-delta.c
> > @@ -162,8 +162,8 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> > hmask = hsize - 1;
> >
> > /* allocate lookup index */
> > - memsize = sizeof(*hash) * hsize +
> > - sizeof(*entry) * entries;
> > + memsize = st_add(st_mult(sizeof(*hash), hsize),
> > + st_mult(sizeof(*entry), entries));
> > mem = malloc(memsize);
> > if (!mem)
> > return NULL;
> > @@ -258,9 +258,9 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> > * Now create the packed index in array form
> > * rather than linked lists.
> > */
> > - memsize = sizeof(*index)
> > - + sizeof(*packed_hash) * (hsize+1)
> > - + sizeof(*packed_entry) * entries;
> > + memsize = st_add3(sizeof(*index),
> > + st_mult(sizeof(*packed_hash), hsize + 1),
> > + st_mult(sizeof(*packed_entry), entries));
> > mem = malloc(memsize);
> > if (!mem) {
> > free(hash);
> >
> > base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
Hi Junio,
Thank you for the detailed correction.
I have expanded the commit message to include the step-by-step
breakdown for both the unpacked and packed index allocations,
detailing the caps on hsize (2^26) and entries (2^28) alongside struct
padding on 32-bit systems. I also corrected the phrasing regarding
(hsize + 1) being passed as an argument to st_add3().
I have updated the patch series accordingly.
Best regards,
Srujith |
Author
|
/submit |
|
Submitted as pull.2276.v4.git.1792864857176.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cc: Phillip Wood phillip.wood123@gmail.com