Repository navigation
Conversation
Welcome to GitGitGadgetHi @msuozzo, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
Invalid author email in 9b2bc74: "1393068+msuozzo@users.noreply.github.com" |
9b2bc74 to
5fb73ec
Compare
|
/allow |
|
User msuozzo is now allowed to use GitGitGadget. WARNING: msuozzo has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use. |
|
/preview |
|
Preview email sent as pull.2255.git.1791989689429.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.2255.git.1791989883899.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Oct 14, 2026 at 02:58:03PM +0000, Matthew Suozzo via GitGitGadget wrote:
> From: Matthew Suozzo <m@suozzo.co>
>
> An object record's count of offsets is a 64-bit varint from the table,
> and reftable_obj_record_decode() allocates the offsets array from it
> through REFTABLE_ALLOC_ARRAY(), which takes a size_t. Where size_t is
> 32 bits, a count of 2^32 + 1 allocates room for one offset, and the
> loop, which runs to the full count, writes an offset from the table
> for each varint left in the block past the end of the array. Where
> size_t is 64 bits, a large count only makes a large allocation that
> the decode then fails to fill.
>
> Nothing in Git reaches this decoder from a repository: a block of a
> type other than the one wanted is refused before its records are
> decoded, and the object index, the only reader of object blocks, is
> used only by the unit tests. The decoder is still wrong.
>
> Each offset takes at least a byte, so refuse a count larger than what
> is left of the record.
Sorry, but this whole commit message reads very AI-generated to me, as
it is both overly verbose and hard to parse. I would welcome it if you
could maybe touch up the message for normal human beings.
> diff --git a/Makefile b/Makefile
> index a96be506b5..66b7270fc0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1551,6 +1551,7 @@ CLAR_TEST_SUITES += u-reftable-block
> CLAR_TEST_SUITES += u-reftable-merged
> CLAR_TEST_SUITES += u-reftable-pq
> CLAR_TEST_SUITES += u-reftable-readwrite
> +CLAR_TEST_SUITES += u-reftable-record
> CLAR_TEST_SUITES += u-reftable-stack
> CLAR_TEST_SUITES += u-reftable-table
> CLAR_TEST_SUITES += u-reftable-tree
Makes sense, even though this should probably have been in its own
patch.
> diff --git a/reftable/record.c b/reftable/record.c
> index 1fce441930..96eceddca7 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -589,6 +589,10 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,
> if (count == 0)
> return start.len - in.len;
>
> + /* each offset takes at least a byte */
> + if (count > in.len)
> + return REFTABLE_FORMAT_ERROR;
This check doesn't really seem to match what you're describing. What you
describe is an integer overflow, but you only check for a very specific
edge ecase here. I would have expected for example that...
> REFTABLE_ALLOC_ARRAY(r->offsets, count);
... this line already detects the overflow and then refuses the
operation. Because `REFTABLE_ALLOC_ARRAY()` itself already tries to
check for overflows.
That being said, if you take a look at `reftable_alloc_size()` you can
see that the input is both of type `size_t`. And _that_ is indeed broken
on platforms where `size_t` is 32 bits, as we'd potentially wrap. But
that fact is not even mentioned as part of the commit message at all.
So overall I'm not convinced that this fix makes sense, we should rather
fix the source of the problem, which is `reftable_alloc_size()`.
Thanks!
Patrick |
|
m@suozzo.co wrote on the Git mailing list (how to reply to this email): October 19, 2026 at 3:46 AM, "Patrick Steinhardt" <ps@pks.im mailto:ps@pks.im?to=%22Patrick%20Steinhardt%22%20%3Cps%40pks.im%3E > wrote:
>
> On Wed, Oct 14, 2026 at 02:58:03PM +0000, Matthew Suozzo via GitGitGadget wrote:
>
> >
> > From: Matthew Suozzo <m@suozzo.co>
> >
> > An object record's count of offsets is a 64-bit varint from the table,
> > and reftable_obj_record_decode() allocates the offsets array from it
> > through REFTABLE_ALLOC_ARRAY(), which takes a size_t. Where size_t is
> > 32 bits, a count of 2^32 + 1 allocates room for one offset, and the
> > loop, which runs to the full count, writes an offset from the table
> > for each varint left in the block past the end of the array. Where
> > size_t is 64 bits, a large count only makes a large allocation that
> > the decode then fails to fill.
> >
> > Nothing in Git reaches this decoder from a repository: a block of a
> > type other than the one wanted is refused before its records are
> > decoded, and the object index, the only reader of object blocks, is
> > used only by the unit tests. The decoder is still wrong.
> >
> > Each offset takes at least a byte, so refuse a count larger than what
> > is left of the record.
> >
> Sorry, but this whole commit message reads very AI-generated to me, as
> it is both overly verbose and hard to parse. I would welcome it if you
> could maybe touch up the message for normal human beings.
yeah it was AI generated and i copy-edited it. it didn't feel too excessive since i wanted to be precise about the issue's mechanism, scope (32-bit only), severity (not really reachable), and describe the fix.
any pointers on how you'd reword? remove the exposition?
>
> >
> > diff --git a/Makefile b/Makefile
> > index a96be506b5..66b7270fc0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1551,6 +1551,7 @@ CLAR_TEST_SUITES += u-reftable-block
> > CLAR_TEST_SUITES += u-reftable-merged
> > CLAR_TEST_SUITES += u-reftable-pq
> > CLAR_TEST_SUITES += u-reftable-readwrite
> > +CLAR_TEST_SUITES += u-reftable-record
> > CLAR_TEST_SUITES += u-reftable-stack
> > CLAR_TEST_SUITES += u-reftable-table
> > CLAR_TEST_SUITES += u-reftable-tree
> >
> Makes sense, even though this should probably have been in its own
> patch.
happy to move it to a separate patch.
>
> >
> > diff --git a/reftable/record.c b/reftable/record.c
> > index 1fce441930..96eceddca7 100644
> > --- a/reftable/record.c
> > +++ b/reftable/record.c
> > @@ -589,6 +589,10 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,
> > if (count == 0)
> > return start.len - in.len;
> >
> > + /* each offset takes at least a byte */
> > + if (count > in.len)
> > + return REFTABLE_FORMAT_ERROR;
> >
> This check doesn't really seem to match what you're describing. What you
> describe is an integer overflow, but you only check for a very specific
> edge ecase here. I would have expected for example that...
>
> >
> > REFTABLE_ALLOC_ARRAY(r->offsets, count);
> >
> ... this line already detects the overflow and then refuses the
> operation. Because `REFTABLE_ALLOC_ARRAY()` itself already tries to
> check for overflows.
>
> That being said, if you take a look at `reftable_alloc_size()` you can
> see that the input is both of type `size_t`. And _that_ is indeed broken
> on platforms where `size_t` is 32 bits, as we'd potentially wrap. But
> that fact is not even mentioned as part of the commit message at all.
isn't that "REFTABLE_ALLOC_ARRAY(), which takes a size_t" in the original message?
>
> So overall I'm not convinced that this fix makes sense, we should rather
> fix the source of the problem, which is `reftable_alloc_size()`.
that felt like a pretty disruptive change i was hesitant to put up as my first patch but i'd be happy to make the change.
>
> Thanks!
>
> Patrick
> |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Mon, Oct 19, 2026 at 05:14:08PM +0000, m@suozzo.co wrote:
> October 19, 2026 at 3:46 AM, "Patrick Steinhardt" <ps@pks.im mailto:ps@pks.im?to=%22Patrick%20Steinhardt%22%20%3Cps%40pks.im%3E > wrote:
> > On Wed, Oct 14, 2026 at 02:58:03PM +0000, Matthew Suozzo via GitGitGadget wrote:
> > > From: Matthew Suozzo <m@suozzo.co>
> > >
> > > An object record's count of offsets is a 64-bit varint from the table,
> > > and reftable_obj_record_decode() allocates the offsets array from it
> > > through REFTABLE_ALLOC_ARRAY(), which takes a size_t. Where size_t is
> > > 32 bits, a count of 2^32 + 1 allocates room for one offset, and the
> > > loop, which runs to the full count, writes an offset from the table
> > > for each varint left in the block past the end of the array. Where
> > > size_t is 64 bits, a large count only makes a large allocation that
> > > the decode then fails to fill.
> > >
> > > Nothing in Git reaches this decoder from a repository: a block of a
> > > type other than the one wanted is refused before its records are
> > > decoded, and the object index, the only reader of object blocks, is
> > > used only by the unit tests. The decoder is still wrong.
> > >
> > > Each offset takes at least a byte, so refuse a count larger than what
> > > is left of the record.
> > >
> > Sorry, but this whole commit message reads very AI-generated to me, as
> > it is both overly verbose and hard to parse. I would welcome it if you
> > could maybe touch up the message for normal human beings.
>
> yeah it was AI generated and i copy-edited it. it didn't feel too
> excessive since i wanted to be precise about the issue's mechanism,
> scope (32-bit only), severity (not really reachable), and describe the
> fix.
>
> any pointers on how you'd reword? remove the exposition?
It's hard to say. With AI-generated commit messages you very often have
the feeling that you end up understanding everything and nothing. It
just feels off in a way that is hard to describe, and consequently it's
also hard to say what exactly needs changing.
My typical suggestion is to not use AI to write your commit message at
all and instead write it yourself. Commit messages have always been the
place where you can demonstrate that you understand what you are doing
as part of the patch by properly explaining it in a way that another
human being understands it. And I think that role is becoming even more
important now: in a world where we cannot trust that the contributor has
the slightest understanding of what they are contributing, they need to
go the extra mile to prove it to us by having a well-written explanation
thereof.
I know that this is putting more of a burden on contributors, and I'm
not a fan of that. But I think it's becoming abundantly clear that AI
does not really give open communities much of a choice, at least not if
they want to keep standards high.
> > > diff --git a/reftable/record.c b/reftable/record.c
> > > index 1fce441930..96eceddca7 100644
> > > --- a/reftable/record.c
> > > +++ b/reftable/record.c
> > > @@ -589,6 +589,10 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,
> > > if (count == 0)
> > > return start.len - in.len;
> > >
> > > + /* each offset takes at least a byte */
> > > + if (count > in.len)
> > > + return REFTABLE_FORMAT_ERROR;
> > >
> > This check doesn't really seem to match what you're describing. What you
> > describe is an integer overflow, but you only check for a very specific
> > edge ecase here. I would have expected for example that...
> >
> > >
> > > REFTABLE_ALLOC_ARRAY(r->offsets, count);
> > >
> > ... this line already detects the overflow and then refuses the
> > operation. Because `REFTABLE_ALLOC_ARRAY()` itself already tries to
> > check for overflows.
> >
> > That being said, if you take a look at `reftable_alloc_size()` you can
> > see that the input is both of type `size_t`. And _that_ is indeed broken
> > on platforms where `size_t` is 32 bits, as we'd potentially wrap. But
> > that fact is not even mentioned as part of the commit message at all.
>
> isn't that "REFTABLE_ALLOC_ARRAY(), which takes a size_t" in the original message?
Ugh, no idea how I missed that. Probably because my brain was sucked out
while reading it.
> > So overall I'm not convinced that this fix makes sense, we should rather
> > fix the source of the problem, which is `reftable_alloc_size()`.
>
> that felt like a pretty disruptive change i was hesitant to put up as
> my first patch but i'd be happy to make the change.
Hm, I wouldn't call it disruptive but rather holistic. By fixing that
one function we also implicitly fix all the other callers that may
currently be wrong. And in the end nothing really changes for callers
anyway: I'd change the parameters to be of type `uintmax_t`, but the
return value should probably remain a `size_t`. So we'd continue to work
just fine in all cases where there is no overflow of `size_t`, but we'd
properly detect cases where the parameters themselves already overflow
it.
Patrick |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Patrick Steinhardt <ps@pks.im> writes:
> It's hard to say. With AI-generated commit messages you very often have
> the feeling that you end up understanding everything and nothing. It
> just feels off in a way that is hard to describe, and consequently it's
> also hard to say what exactly needs changing.
>
> My typical suggestion is to not use AI to write your commit message at
> all and instead write it yourself. Commit messages have always been the
> place where you can demonstrate that you understand what you are doing
> as part of the patch by properly explaining it in a way that another
> human being understands it. And I think that role is becoming even more
> important now: in a world where we cannot trust that the contributor has
> the slightest understanding of what they are contributing, they need to
> go the extra mile to prove it to us by having a well-written explanation
> thereof.
>
> I know that this is putting more of a burden on contributors, and I'm
> not a fan of that. But I think it's becoming abundantly clear that AI
> does not really give open communities much of a choice, at least not if
> they want to keep standards high.
Putting more burden on the user is fine, and the AI output, when
used correctly, can be not so bad.
Having said that, the real problem is that nobody seems to guide
generative AI machinery well enough to make intelligible output. In
order for a user to guide the machinery well enough, they must know
what they want to discuss, and the output would be different from
what a clueless and hands-off "here is a bug report from somewhere I
saw, I do not understand the system well enough, solve it and
describe the solution" prompt would result in.
Writing the first draft manually and having a machine check and
correct typos and grammar is not a bad practice, as you remain in
control of the overall structure and contents of the final result.
Going in the other direction? If you are not familiar with the area
and are easily fooled by intelligent-sounding nonsense uttered by
generative AI, correcting such nonsense into a usable shape would be
difficult, and would make those on the receiving end who have to
read it miserable.
By the way, I cannot help but wonder what happened to the AI policy
discussion that the contributors' summit promised we would have
soon. Who is organizing it? |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Oct 20, 2026 at 08:08:01AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> By the way, I cannot help but wonder what happened to the AI policy
> discussion that the contributors' summit promised we would have
> soon. Who is organizing it?
I think Emily wanted to kick it off, if I'm not mistaken.
Patrick |
|
Aniket Gupta wrote on the Git mailing list (how to reply to this email): Hi Matthew,
> By fixing that one function we also implicitly fix all the other
> callers that may currently be wrong.
I tried Patrick's change on a 32 bit build and it works for
REFTABLE_ALLOC_ARRAY. However REFTABLE_CALLOC_ARRAY goes straight to
reftable_calloc() with a size_t and gets truncated. I think no one
passes a 64 bit count to it so nothing breaks but maybe worth a
mention in the commit message?
Also the new test passes because of the count > in.len check. If
that check goes away in v2 I think it might need a different test
since on 64 bit nothing truncates. Maybe we can test
reftable_alloc_size() directly?
Aniket |
|
User |
The Meson build includes the reftable record unit tests, but the Makefile does not. The suite was dropped from the Makefile when the test was converted to the clar framework in 2596bef (2025-07-24). Include the reftable record unit test suite in the Makefile test target. Assisted-by: Claude Fable 5.1 Signed-off-by: Matthew Suozzo <m@suozzo.co>
reftable_alloc_size() takes its arguments as size_t, but reftable_obj_record_decode() passes it a uint64_t count. On 32-bit platforms, a count wider than 32 bits is truncated before the overflow check, so the decoder can get a small allocation for a large count and write past its end. reftable_calloc() takes size_t arguments the same way. Make reftable_alloc_size() and reftable_calloc() take uintmax_t arguments, so the check sees the full count. Assisted-by: Claude Opus 5.5 Helped-by: Patrick Steinhardt <ps@pks.im> Helped-by: Aniket Gupta <aniketgupta3001@gmail.com> Signed-off-by: Matthew Suozzo <m@suozzo.co>
5fb73ec to
8f4a838
Compare
msuozzo
changed the title
|
/preview |
|
Preview email sent as pull.2255.v2.git.1792614713.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.2255.v2.git.1792616615.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@74d7c17. |
|
There was a status update in the "New Topics" section about the branch The allocation size validation and memory allocation functions in the reftable backend have been updated to use 'uintmax_t' for their size and element count arguments. This prevents wide values from being truncated to 'size_t' on 32-bit platforms before the allocation size overflow check is performed. Needs review. source: <pull.2255.v2.git.1792616615.gitgitgadget@gmail.com> |
Changes since v1:
instead of checking the count in reftable_obj_record_decode()
(Patrick, Aniket).
cc: Patrick Steinhardt ps@pks.im
cc: Aniket Gupta aniketgupta3001@gmail.com