{"thread":{"id":"64062","subject":"[PATCH] add-interactive: reject malformed numerical input","startedAt":"2025-08-30T11:31:38Z","lastAt":"2025-09-08T13:31:45Z","messageCount":5,"participants":["Seonghyeon Cho (조성현) via GitGitGadget","Patrick Steinhardt","Seonghyeon Cho"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"525250","messageId":"pull.2044.git.git.1756553495661.gitgitgadget@gmail.com","threadId":"64062","inReplyTo":null,"subject":"[PATCH] add-interactive: reject malformed numerical input","fromName":"Seonghyeon Cho (조성현) via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-30T11:31:35Z","receivedAt":"2025-08-30T11:31:38Z","isPatch":true,"sender":{"key":"name:Seonghyeon Cho (조성현)","avatar":null},"body":"From: Seonghyeon Cho \n\nThe list-and-choose interface accepts malformed input such as \"2m3\" and\ninterprets it as \"2-\", silently selecting a range to the end. This is\nmisleading and makes it easy to select unintended items.\n\nReject such input by treating it as invalid.\n\nSigned-off-by: Seonghyeon Cho \n---\n add-interactive: reject malformed numerical input\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2044%2Fsh-cho%2Freject-malformed-input-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2044/sh-cho/reject-malformed-input-v1\nPull-Request: https://github.com/git/git/pull/2044\n\n add-interactive.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 3e692b47ec..86ff632288 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -396,6 +396,8 @@ static ssize_t list_and_choose(struct add_i_state *s,\n \t\t\t\t\tif (endp != p + sep)\n \t\t\t\t\t\tfrom = -1;\n \t\t\t\t}\n+\t\t\t\telse\n+\t\t\t\t\tfrom = -1;\n \t\t\t}\n \n \t\t\tif (p[sep])\n\nbase-commit: 6ad802182101d622e6a4132f48292ddfa79e2024\n-- \ngitgitgadget\n"},{"id":"525309","messageId":"aLaz7yCXWGG2_oP_@pks.im","threadId":"64062","inReplyTo":"pull.2044.git.git.1756553495661.gitgitgadget@gmail.com","subject":"Re: [PATCH] add-interactive: reject malformed numerical input","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-02T09:07:59Z","receivedAt":"2025-09-02T09:08:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 30, 2025 at 11:31:35AM +0000, Seonghyeon Cho (조성현) via GitGitGadget wrote:\n> From: Seonghyeon Cho \n> \n> The list-and-choose interface accepts malformed input such as \"2m3\" and\n> interprets it as \"2-\", silently selecting a range to the end. This is\n> misleading and makes it easy to select unintended items.\n> \n> Reject such input by treating it as invalid.\n\nOkay, that does feel fishy indeed. It would be good though to have a\ntest case that demonstrates the new behaviour and at the same time\nensures that we don't regress in the future. You can have a look at\n\"t3701-add-interactive.sh\", which has a bunch of other tests for this\ncommand, as well.\n\nIn general though we're not doing a good job here of error checking. We\ndon't at all verify whether `strtoul()` returned an error, for example\nERANGE. So if a user passes an integer that exceeds whatever we can\nstore in an `unsigned long` we'll silently proceed with a bogus result,\nwon't we?\n\nIdeally, we'd use a saner interface to parse these integers, like for\nexample our own `git_parse_ulong()`. But unfortunately, that interface\ndoes not handle the case where we only want to parse a substring in a\nlonger string. Too bad.\n\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 3e692b47ec..86ff632288 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -396,6 +396,8 @@ static ssize_t list_and_choose(struct add_i_state *s,\n> \t\t\t\t\tif (endp != p + sep)\n> \t\t\t\t\t\tfrom = -1;\n> \t\t\t\t}\n> +\t\t\t\telse\n> +\t\t\t\t\tfrom = -1;\n> \t\t\t}\n\nCoding style: the `else` should sit on the same line as the closing\ncurly brace. And furthermore, if one of the branches of an if-else chain\nrequires curly braces, then all branches should have curly braces.\n\nPatrick\n"},{"id":"525707","messageId":"aL15aTmKQOsNrF0D@Seonghyeons-Mac-Studio.local","threadId":"64062","inReplyTo":"aLaz7yCXWGG2_oP_@pks.im","subject":"Re: [PATCH] add-interactive: reject malformed numerical input","fromName":"Seonghyeon Cho","fromEmail":"seonghyeoncho96@gmail.com","sentAt":"2025-09-07T12:24:09Z","receivedAt":"2025-09-07T12:24:16Z","isPatch":true,"sender":{"key":"seonghyeoncho96@gmail.com","avatar":null},"body":"On Tue, Sep 02, 2025 at 11:07:59AM +0200, Patrick Steinhardt wrote:\n> Okay, that does feel fishy indeed. It would be good though to have a\n> test case that demonstrates the new behaviour and at the same time\n> ensures that we don't regress in the future. You can have a look at\n> \"t3701-add-interactive.sh\", which has a bunch of other tests for this\n> command, as well.\n\nOkay, I'll add tests.\n\n> In general though we're not doing a good job here of error checking. We\n> don't at all verify whether `strtoul()` returned an error, for example\n> ERANGE. So if a user passes an integer that exceeds whatever we can\n> store in an `unsigned long` we'll silently proceed with a bogus result,\n> won't we?\n> \n> Ideally, we'd use a saner interface to parse these integers, like for\n> example our own `git_parse_ulong()`. But unfortunately, that interface\n> does not handle the case where we only want to parse a substring in a\n> longer string. Too bad.\n\nGood point. Would you prefer I introduce new parse method here, or\nshould this be handled in separate patch?\n\n> Coding style: the `else` should sit on the same line as the closing\n> curly brace. And furthermore, if one of the branches of an if-else chain\n> requires curly braces, then all branches should have curly braces.\n\nOk, I'll fix coding styles.\n\nThanks,\nSeonghyeon\n\n"},{"id":"525747","messageId":"aL5VxjPSqfXbnY7W@pks.im","threadId":"64062","inReplyTo":"aL15aTmKQOsNrF0D@Seonghyeons-Mac-Studio.local","subject":"Re: [PATCH] add-interactive: reject malformed numerical input","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-08T04:04:22Z","receivedAt":"2025-09-08T04:04:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Sep 07, 2025 at 09:24:09PM +0900, Seonghyeon Cho wrote:\n> On Tue, Sep 02, 2025 at 11:07:59AM +0200, Patrick Steinhardt wrote:\n> > In general though we're not doing a good job here of error checking. We\n> > don't at all verify whether `strtoul()` returned an error, for example\n> > ERANGE. So if a user passes an integer that exceeds whatever we can\n> > store in an `unsigned long` we'll silently proceed with a bogus result,\n> > won't we?\n> > \n> > Ideally, we'd use a saner interface to parse these integers, like for\n> > example our own `git_parse_ulong()`. But unfortunately, that interface\n> > does not handle the case where we only want to parse a substring in a\n> > longer string. Too bad.\n> \n> Good point. Would you prefer I introduce new parse method here, or\n> should this be handled in separate patch?\n\nI don't think that would need to be part of your patch series. But we\nshould have proper error checking for `strtoul()` if we're already\nimproving this code.\n\nPatrick\n"},{"id":"525800","messageId":"aL7au7hI_zuI7bhW@Seonghyeons-Mac-Studio.local","threadId":"64062","inReplyTo":"aL5VxjPSqfXbnY7W@pks.im","subject":"Re: [PATCH] add-interactive: reject malformed numerical input","fromName":"Seonghyeon Cho","fromEmail":"seonghyeoncho96@gmail.com","sentAt":"2025-09-08T13:31:39Z","receivedAt":"2025-09-08T13:31:45Z","isPatch":true,"sender":{"key":"seonghyeoncho96@gmail.com","avatar":null},"body":"On Mon, Sep 08, 2025 at 06:04:22AM +0200, Patrick Steinhardt wrote:\n> I don't think that would need to be part of your patch series. But we\n> should have proper error checking for `strtoul()` if we're already\n> improving this code.\n\nUnderstood. I'll handle it too.\n\nThanks,\nSeonghyeon\n\n"}]}