threads / patch / 64062

patchadd-interactive: reject malformed numerical input

Subject: [PATCH] add-interactive: reject malformed numerical input

## tl;dr

5 messages between Aug 30, 2025 and Sep 8, 2025. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Seonghyeon Cho (조성현) via GitGitGadget· Aug 30, 2025, 11:31 UTC · lore

From: Seonghyeon Cho <seonghyeoncho96@gmail.com>

The list-and-choose interface accepts malformed input such as "2m3" and interprets it as "2-", silently selecting a range to the end. This is misleading and makes it easy to select unintended items.

Reject such input by treating it as invalid.

Signed-off-by: Seonghyeon Cho <seonghyeoncho96@gmail.com>
---
    add-interactive: reject malformed numerical input

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2044%2Fsh-cho%2Freject-malformed-input-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2044/sh-cho/reject-malformed-input-v1
Pull-Request: https://github.com/git/git/pull/2044

 add-interactive.c | 2 ++
 1 file changed, 2 insertions(+)

Show changes to add-interactive.c +2 −0

diff --git a/add-interactive.c b/add-interactive.c
index 3e692b47ec..86ff632288 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -396,6 +396,8 @@ static ssize_t list_and_choose(struct add_i_state *s,
 					if (endp != p + sep)
 						from = -1;
 				}
+				else
+					from = -1;
 			}
 
 			if (p[sep])

base-commit: 6ad802182101d622e6a4132f48292ddfa79e2024

-- 
gitgitgadget

Patrick Steinhardt· Sep 2, 2025, 09:07 UTC · re: Seonghyeon Cho (조성현) via GitGitGadget · lore

Re: [PATCH] add-interactive: reject malformed numerical input

On Sat, Aug 30, 2025 at 11:31:35AM +0000, Seonghyeon Cho (조성현) via GitGitGadget wrote:

Show 7 quoted lines

> From: Seonghyeon Cho <seonghyeoncho96@gmail.com>
> 
> The list-and-choose interface accepts malformed input such as "2m3" and
> interprets it as "2-", silently selecting a range to the end. This is
> misleading and makes it easy to select unintended items.
> 
> Reject such input by treating it as invalid.

Okay, that does feel fishy indeed. It would be good though to have a test case that demonstrates the new behaviour and at the same time ensures that we don't regress in the future. You can have a look at "t3701-add-interactive.sh", which has a bunch of other tests for this command, as well.

In general though we're not doing a good job here of error checking. We don't at all verify whether `strtoul()` returned an error, for example ERANGE. So if a user passes an integer that exceeds whatever we can store in an `unsigned long` we'll silently proceed with a bogus result, won't we?

Ideally, we'd use a saner interface to parse these integers, like for example our own `git_parse_ulong()`. But unfortunately, that interface does not handle the case where we only want to parse a substring in a longer string. Too bad.

Show 11 quoted lines

> diff --git a/add-interactive.c b/add-interactive.c
> index 3e692b47ec..86ff632288 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -396,6 +396,8 @@ static ssize_t list_and_choose(struct add_i_state *s,
>  					if (endp != p + sep)
>  						from = -1;
>  				}
> +				else
> +					from = -1;
>  			}

Coding style: the `else` should sit on the same line as the closing curly brace. And furthermore, if one of the branches of an if-else chain requires curly braces, then all branches should have curly braces.

Patrick

Seonghyeon Cho· Sep 7, 2025, 12:24 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] add-interactive: reject malformed numerical input

On Tue, Sep 02, 2025 at 11:07:59AM +0200, Patrick Steinhardt wrote:

Show 5 quoted lines

> Okay, that does feel fishy indeed. It would be good though to have a
> test case that demonstrates the new behaviour and at the same time
> ensures that we don't regress in the future. You can have a look at
> "t3701-add-interactive.sh", which has a bunch of other tests for this
> command, as well.

Okay, I'll add tests.

Show 10 quoted lines

> In general though we're not doing a good job here of error checking. We
> don't at all verify whether `strtoul()` returned an error, for example
> ERANGE. So if a user passes an integer that exceeds whatever we can
> store in an `unsigned long` we'll silently proceed with a bogus result,
> won't we?
> 
> Ideally, we'd use a saner interface to parse these integers, like for
> example our own `git_parse_ulong()`. But unfortunately, that interface
> does not handle the case where we only want to parse a substring in a
> longer string. Too bad.

Good point. Would you prefer I introduce new parse method here, or should this be handled in separate patch?

> Coding style: the `else` should sit on the same line as the closing
> curly brace. And furthermore, if one of the branches of an if-else chain
> requires curly braces, then all branches should have curly braces.

Ok, I'll fix coding styles.

Thanks, Seonghyeon

Patrick Steinhardt· Sep 8, 2025, 04:04 UTC · re: Seonghyeon Cho · lore

Re: [PATCH] add-interactive: reject malformed numerical input

On Sun, Sep 07, 2025 at 09:24:09PM +0900, Seonghyeon Cho wrote:

Show 14 quoted lines

> On Tue, Sep 02, 2025 at 11:07:59AM +0200, Patrick Steinhardt wrote:
> > In general though we're not doing a good job here of error checking. We
> > don't at all verify whether `strtoul()` returned an error, for example
> > ERANGE. So if a user passes an integer that exceeds whatever we can
> > store in an `unsigned long` we'll silently proceed with a bogus result,
> > won't we?
> > 
> > Ideally, we'd use a saner interface to parse these integers, like for
> > example our own `git_parse_ulong()`. But unfortunately, that interface
> > does not handle the case where we only want to parse a substring in a
> > longer string. Too bad.
> 
> Good point. Would you prefer I introduce new parse method here, or
> should this be handled in separate patch?

I don't think that would need to be part of your patch series. But we should have proper error checking for `strtoul()` if we're already improving this code.

Patrick

Seonghyeon Cho· Sep 8, 2025, 13:31 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] add-interactive: reject malformed numerical input

On Mon, Sep 08, 2025 at 06:04:22AM +0200, Patrick Steinhardt wrote:

> I don't think that would need to be part of your patch series. But we
> should have proper error checking for `strtoul()` if we're already
> improving this code.

Understood. I'll handle it too.

Thanks, Seonghyeon