Re: [PATCH v6 2/2] patch-id: replace `atoi()` with `strtoi_with_tail()`

Re: [PATCH v6 2/2] patch-id: replace `atoi()` with `strtoi_with_tail()`

[Mohit Marathe]

Hi,

I am writing to inquire about the status of this patch. 

As a contributor, I am curious whether the decision not 
to merge this patch was intentional or if it might have been 
overlooked. Could you kindly provide some clarity on this matter?
I tried finding this on "What's cooking in Git" but couldn't find it.

If there are any specific reasons for not merging the patch, I would 
be grateful if you could share them. Additionally, if there are any 
further corrections needed please do let me know.

Thanks,
Mohit

Re: [PATCH v6 2/2] patch-id: replace `atoi()` with `strtoi_with_tail()`

[Junio C Hamano]

Mohit Marathe <mohitmarathe@proton.me> writes:

> I am writing to inquire about the status of this patch. 

Thanks for pinging.

Please be kind to fellow project members by including a URL under
https://lore.kernel.org/git/ to the original discussion.  In general
there needs to be a reason why a patch should be applied, but a lack
of reason why a patch should be applied is good enough reason why a
patch may not have been applied so far.  I cannot offhand recall
what problems the patch had or if there were additional problems in
the patch that was left unaddressed.

Thanks.

Re: [PATCH v6 2/2] patch-id: replace `atoi()` with `strtoi_with_tail()`

[Mohit Marathe]

> Mohit Marathe mohitmarathe@proton.me writes:
> 
> > I am writing to inquire about the status of this patch.
> 
> 
> Thanks for pinging.
> 
> Please be kind to fellow project members by including a URL under
> https://lore.kernel.org/git/ to the original discussion. In general

I thought replying in the same thread as that of patch would be enough.
But now that I think about it, threads can get pretty long. My bad,I 
will keep in mind to include the URL of the patch next time.

Just for the record, I was talking about this patch:
https://lore.kernel.org/git/pull.1646.v6.git.1707025718.gitgitgadget
@gmail.com/

> there needs to be a reason why a patch should be applied, but a lack
> of reason why a patch should be applied is good enough reason why a
> patch may not have been applied so far. I cannot offhand recall
> what problems the patch had or if there were additional problems in
> the patch that was left unaddressed.
> 
> Thanks.

This makes perfect sense. Thanks for clarifying.

Thanks.

Re: [PATCH v6 2/2] patch-id: replace `atoi()` with `strtoi_with_tail()`

[Junio C Hamano]

Mohit Marathe <mohitmarathe@proton.me> writes:

> I thought replying in the same thread as that of patch would be enough.

Well, if you look at the headers of your message, there only is its
own Message-ID and no In-Reply-To or References, which some/many
MUAs go by.

> Just for the record, I was talking about this patch:
> https://lore.kernel.org/git/pull.1646.v6.git.1707025718.gitgitgadget
> @gmail.com/

Yup, thanks.  I ended up at the same place with subject search ;-)

[PATCH v5 0/2] Replace atoi() with strtoi_with_tail()

[Mohit Marathe via GitGitGadget]

Hello,

This patch series replaces atoi() with an updated version of strtol_i()
called strtoi_with_tail (Credits: Junio C Hamano). The reasoning behind this
is to improve error handling by not allowing non-numerical characters in the
hunk header (which might happen in case of a corrupt patch, although
rarely).

There is still a change to be made, as Junio says: "A corrupt patch may be
getting a nonsense patch-ID with the current code and hopefully is not
matching other patches that are not corrupt, but with such a change, a
corrupt patch may not be getting any patch-ID and a loop that computes
patch-ID for many files and try to match them up might need to be rewritten
to take the new failure case into account." I'm not sure where this change
needs to me made (maybe get_one_patchid()?). It would be great if anyone
could point me to the correct place.

Thanks, Mohit Marathe

Mohit Marathe (2):
  git-compat-util: add strtoi_with_tail()
  patch-id: replace `atoi()` with `strtoi_with_tail`

 builtin/patch-id.c | 12 ++++++++----
 git-compat-util.h  | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)


base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1646%2Fmohit-marathe%2Fupdate-strtol_i-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1646/mohit-marathe/update-strtol_i-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1646

Range-diff vs v4:

 1:  60ea85a701a ! 1:  f09b0838f04 git-compat-util: add strtol_i_updated()
     @@ Metadata
      Author: Mohit Marathe <mohitmarathe23@gmail.com>
      
       ## Commit message ##
     -    git-compat-util: add strtol_i_updated()
     +    git-compat-util: add strtoi_with_tail()
      
          This function is an updated version of strtol_i() function. It will
          give more control to handle parsing of the characters after the
     -    integer and better error handling while parsing numbers.
     +    numbers and better error handling while parsing numbers.
      
          Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
      
     @@ git-compat-util.h: static inline int strtol_i(char const *s, int base, int *resu
       	return 0;
       }
       
     -+#define strtol_i(s,b,r) strtol_i_updated((s), (b), (r), NULL)
     -+static inline int strtol_i_updated(char const *s, int base, int *result, char **endp)
     ++#define strtol_i(s,b,r) strtoi_with_tail((s), (b), (r), NULL)
     ++static inline int strtoi_with_tail(char const *s, int base, int *result, char **endp)
      +{
      +	long ul;
      +	char *dummy = NULL;
 2:  17f2dda4907 ! 2:  ee8f4ae991d patch-id: replace `atoi()` with `strtol_i_updated()`
     @@ Metadata
      Author: Mohit Marathe <mohitmarathe23@gmail.com>
      
       ## Commit message ##
     -    patch-id: replace `atoi()` with `strtol_i_updated()`
     +    patch-id: replace `atoi()` with `strtoi_with_tail`
      
          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
     +    during the conversion of string to integers. The
     +    `strtoi_with_tail` 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
     +    detection. Unlike `atoi`, `strtoi_with_tail` 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.
     @@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
       	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)
     +-		n = strspn(q, digits);
     ++		if (strtoi_with_tail(q, 10, p_before, &endp) != 0)
      +			return 0;
     ++		n = endp - q;
       	} else {
       		*p_before = 1;
       	}
     @@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
       	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)
     +-		n = strspn(r, digits);
     ++		if (strtoi_with_tail(r, 10, p_after, &endp) != 0)
      +			return 0;
     ++		n = endp - r;
       	} else {
       		*p_after = 1;
       	}

-- 
gitgitgadget

[PATCH v6 0/2] Replace atoi() with strtoi_with_tail()

[Mohit Marathe via GitGitGadget]

Hello,

This patch series replaces atoi() with an updated version of strtol_i()
called strtoi_with_tail (Credits: Junio C Hamano). The reasoning behind this
is to improve error handling by not allowing non-numerical characters in the
hunk header (which might happen in case of a corrupt patch, although
rarely).

There is still a change to be made, as Junio says: "A corrupt patch may be
getting a nonsense patch-ID with the current code and hopefully is not
matching other patches that are not corrupt, but with such a change, a
corrupt patch may not be getting any patch-ID and a loop that computes
patch-ID for many files and try to match them up might need to be rewritten
to take the new failure case into account." I'm not sure where this change
needs to me made (maybe get_one_patchid()?). It would be great if anyone
could point me to the correct place.

Thanks, Mohit Marathe

Mohit Marathe (2):
  git-compat-util: add strtoi_with_tail()
  patch-id: replace `atoi()` with `strtoi_with_tail`

 builtin/patch-id.c | 12 ++++++++----
 git-compat-util.h  | 18 ++++++++++++++----
 2 files changed, 22 insertions(+), 8 deletions(-)


base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1646%2Fmohit-marathe%2Fupdate-strtol_i-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1646/mohit-marathe/update-strtol_i-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1646

Range-diff vs v5:

 1:  f09b0838f04 ! 1:  98e516a7be7 git-compat-util: add strtoi_with_tail()
     @@ Commit message
          Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
      
       ## git-compat-util.h ##
     -@@ git-compat-util.h: static inline int strtol_i(char const *s, int base, int *result)
     +@@ git-compat-util.h: static inline int strtoul_ui(char const *s, int base, unsigned int *result)
       	return 0;
       }
       
     +-static inline int strtol_i(char const *s, int base, int *result)
      +#define strtol_i(s,b,r) strtoi_with_tail((s), (b), (r), NULL)
      +static inline int strtoi_with_tail(char const *s, int base, int *result, char **endp)
     -+{
     -+	long ul;
     + {
     + 	long ul;
     +-	char *p;
      +	char *dummy = NULL;
     -+
     + 
      +	if (!endp)
      +		endp = &dummy;
     -+	errno = 0;
     + 	errno = 0;
     +-	ul = strtol(s, &p, base);
     +-	if (errno || *p || p == s || (int) ul != ul)
      +	ul = strtol(s, endp, base);
      +	if (errno ||
      +	    /*
     @@ git-compat-util.h: static inline int strtol_i(char const *s, int base, int *resu
      +	     */
      +	   (dummy && *dummy) ||
      +	    *endp == s || (int) ul != ul)
     -+		return -1;
     -+	*result = ul;
     -+	return 0;
     -+}
     -+
     - void git_stable_qsort(void *base, size_t nmemb, size_t size,
     - 		      int(*compar)(const void *, const void *));
     - #ifdef INTERNAL_QSORT
     + 		return -1;
     + 	*result = ul;
     + 	return 0;
 2:  ee8f4ae991d = 2:  858d6f94e79 patch-id: replace `atoi()` with `strtoi_with_tail`

-- 
gitgitgadget

[PATCH v6 2/2] patch-id: replace `atoi()` with `strtoi_with_tail`

[Mohit Marathe via GitGitGadget]

From: Mohit Marathe <mohitmarathe23@gmail.com>

The change is made to improve the error-handling capabilities
during the conversion of string to integers. The
`strtoi_with_tail` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
detection. Unlike `atoi`, `strtoi_with_tail` 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 <mohitmarathe@proton.me>
---
 builtin/patch-id.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3894d2b9706..4e9a301e9fb 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,16 @@ 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);
-		n = strspn(q, digits);
+		if (strtoi_with_tail(q, 10, p_before, &endp) != 0)
+			return 0;
+		n = endp - q;
 	} else {
 		*p_before = 1;
 	}
@@ -48,8 +51,9 @@ 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);
-		n = strspn(r, digits);
+		if (strtoi_with_tail(r, 10, p_after, &endp) != 0)
+			return 0;
+		n = endp - r;
 	} else {
 		*p_after = 1;
 	}
-- 
gitgitgadget