From: Mohit Marathe <mohitmarathe@proton.me>
Date: Sun, 28 Jan 2024 04:35:02 GMT
Subject: Re: [PATCH v4 2/2] patch-id: replace `atoi()` with `strtol_i_updated()`
Message-ID: <0d_yXUwMsJQsIySLnVJ5SlpFi2YdTgnvVN5cLCbwt_V4pVOUipzFEPlFY1xLicHfe4SGsWwF5D1q1idGBVyRXXr6tME-GR6hQG7usi5mypw=@proton.me>
In-Reply-To: <xmqqttn2bg2o.fsf@gitster.g>


On Thursday, January 25th, 2024 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:

> "Mohit Marathe via GitGitGadget" gitgitgadget@gmail.com writes:
> 
> > q = p + 4;
> > n = strspn(q, digits);
> > if (q[n] == ',') {
> > q += n + 1;
> 
> 
> So, we saw "@@ -" and skipped over these four bytes, skipped the
> digits from there, and found a comma.
> 
> For "@@ -29,14 +30,18 @@", for example, our q is now "14 +30,18 @@"
> as we have skipped over that comma after 29.
> 
> > - *p_before = atoi(q);
> > + if (strtol_i_updated(q, 10, p_before, &endp) != 0)
> > + return 0;
> 
> 
> We parse out 14 and store it to *p_before. endp points at " +30..."
> now.
> 
> > n = strspn(q, digits);
> > + if (endp != q + n)
> > + return 0;
> 
> 
> Is this necessary? By asking strtol_i_updated() where the number ended,
> we already know endp without skipping the digits in q with strspn().
> Shouldn't these three lines become more like
> 
> n = endp - q;
> 
> instead?
> 
> After all, we are not trying to find a bug in strtol_i_updated(),
> which would be the only reason how this "return 0" would trigger.
> 

I was confused about how an invalid hunk header of a corrupted would
look like. This was just an attempt of making a sanity check. But after
taking another look, I agree that its unnecessary.

> > } else {
> > *p_before = 1;
> > }
> > @@ -48,8 +53,11 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
> > n = strspn(r, digits);
> > if (r[n] == ',') {
> > r += n + 1;
> > - *p_after = atoi(r);
> > + if (strtol_i_updated(r, 10, p_after, &endp) != 0)
> > + return 0;
> > n = strspn(r, digits);
> > + if (endp != r + n)
> > + return 0;
> 
> 
> Likewise.
> 
> > } else {
> > *p_after = 1;
> > }