[v4,2/2] patch-id: replace `atoi()` with `strtol_i_updated()`

Commit Message

Mohit Marathe Jan. 24, 2024, 6:55 a.m. UTC

From: Mohit Marathe <[e-mail]>

The change is made to improve the error-handling capabilities
during the conversion of string representations to integers.
The `strtol_i_updated(` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
detection. Unlike `atoi(`, `strtol_i_updated(` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.

Signed-off-by: Mohit Marathe <[e-mail]>
---
 builtin/patch-id.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Jan. 24, 2024, 9:02 p.m. UTC | #1

"Mohit Marathe via GitGitGadget" <[e-mail]> 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.

>  	} 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;
>  	}

Mohit Marathe Jan. 28, 2024, 4:35 a.m. UTC | #2

On Thursday, January 25th, 2024 at 2:32 AM, Junio C Hamano <[e-mail]> wrote:

> "Mohit Marathe via GitGitGadget" [e-mail] 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;
> > }

Patch

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3894d2b9706..88db178c905 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,3 +1,4 @@ 
+#include "git-compat-util.h"
 #include "builtin.h"
 #include "config.h"
 #include "diff.h"
@@ -29,14 +30,18 @@  static int scan_hunk_header(const char *p, int *p_before, int *p_after)
 {
 	static const char digits[] = "0123456789";
 	const char *q, *r;
+	char *endp;
 	int n;
 
 	q = p + 4;
 	n = strspn(q, digits);
 	if (q[n] == ',') {
 		q += n + 1;
-		*p_before = atoi(q);
+		if (strtol_i_updated(q, 10, p_before, &endp) != 0)
+			return 0;
 		n = strspn(q, digits);
+		if (endp != q + n)
+			return 0;
 	} 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;
 	} else {
 		*p_after = 1;
 	}