diff mbox series

[v3,1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

Message ID e292b82d6a1d46990477a043901fa9c56bc00023.1729574624.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series parse: replace atoi() with strtoul_ui() and strtol_i() | expand

Commit Message

Usman Akinyemi Oct. 22, 2024, 5:23 a.m. UTC 
From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Replace atoi() with strtoul_ui() for --timeout and --init-timeout
(non-negative integers) and with strtol_i() for --max-connections
(signed integers). This improves error handling and input validation
by detecting invalid values and providing clear error messages.
Update tests to ensure these arguments are properly validated.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 daemon.c              | 11 +++++++----
 t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

Comments

Taylor Blau Oct. 22, 2024, 4:21 p.m. UTC | #1 
On Tue, Oct 22, 2024 at 05:23:41AM +0000, Usman Akinyemi via GitGitGadget wrote:
> diff --git a/daemon.c b/daemon.c
> index cb946e3c95f..09a31d2344d 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1308,17 +1308,20 @@ int cmd_main(int argc, const char **argv)
>  			continue;
>  		}
>  		if (skip_prefix(arg, "--timeout=", &v)) {
> -			timeout = atoi(v);
> +			if (strtoul_ui(v, 10, &timeout))
> +				die("invalid timeout '%s', expecting a non-negative integer", v);

The conversion you made to both (a) use warning() and (b) mark the
string for translation in the second patch were good, but I would have
expected to see them here as well.

Perhaps leaving this one as a die() makes sense, because we are taking
direct input from the user, so invoking 'git daemon' with bogus options
should result in us dying. But these strings should be marked as
translate-able regardless.

Thanks,
Taylor
Usman Akinyemi Oct. 22, 2024, 10:06 p.m. UTC | #2 
On Tue, Oct 22, 2024 at 4:21 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Oct 22, 2024 at 05:23:41AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > diff --git a/daemon.c b/daemon.c
> > index cb946e3c95f..09a31d2344d 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1308,17 +1308,20 @@ int cmd_main(int argc, const char **argv)
> >                       continue;
> >               }
> >               if (skip_prefix(arg, "--timeout=", &v)) {
> > -                     timeout = atoi(v);
> > +                     if (strtoul_ui(v, 10, &timeout))
> > +                             die("invalid timeout '%s', expecting a non-negative integer", v);
>
> The conversion you made to both (a) use warning() and (b) mark the
> string for translation in the second patch were good, but I would have
> expected to see them here as well.
>
> Perhaps leaving this one as a die() makes sense, because we are taking
> direct input from the user, so invoking 'git daemon' with bogus options
> should result in us dying. But these strings should be marked as
> translate-able regardless.
As you said, since the git daemon takes direct input from the user,
compared to the other which takes input from .gitattributes leaving as
die is okay here. I have marked it as translate-able in my fourth
patch. Thank you very much for the review.
Usman Akinyemi.
>
> Thanks,
> Taylor
diff mbox series

Patch

diff --git a/daemon.c b/daemon.c
index cb946e3c95f..09a31d2344d 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1308,17 +1308,20 @@  int cmd_main(int argc, const char **argv)
 			continue;
 		}
 		if (skip_prefix(arg, "--timeout=", &v)) {
-			timeout = atoi(v);
+			if (strtoul_ui(v, 10, &timeout))
+				die("invalid timeout '%s', expecting a non-negative integer", v);
 			continue;
 		}
 		if (skip_prefix(arg, "--init-timeout=", &v)) {
-			init_timeout = atoi(v);
+			if (strtoul_ui(v, 10, &init_timeout))
+				die("invalid init-timeout '%s', expecting a non-negative integer", v);
 			continue;
 		}
 		if (skip_prefix(arg, "--max-connections=", &v)) {
-			max_connections = atoi(v);
+			if (strtol_i(v, 10, &max_connections))
+				die("invalid max-connections '%s', expecting an integer", v);
 			if (max_connections < 0)
-				max_connections = 0;	        /* unlimited */
+				max_connections = 0;  /* unlimited */
 			continue;
 		}
 		if (!strcmp(arg, "--strict-paths")) {
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index c5f08b67996..722ddb8b7fa 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -8,6 +8,32 @@  TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+test_expect_success 'daemon rejects invalid --init-timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --init-timeout="$arg" 2>actual_error &&
+		test_write_lines "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
+		test_cmp actual_error expected || return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --timeout values' '
+	for arg in "3a" "-3"
+	do
+		test_must_fail git daemon --timeout="$arg" 2>actual_error &&
+		test_write_lines "fatal: invalid timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expected &&
+		test_cmp actual_error expected || return 1
+	done
+'
+
+test_expect_success 'daemon rejects invalid --max-connections values' '
+	arg='3a' &&
+	test_must_fail git daemon --max-connections=3a 2>actual_error &&
+	test_write_lines "fatal: invalid max-connections ${SQ}$arg${SQ}, expecting an integer" >expected &&
+	test_cmp actual_error expected
+'
+
 start_git_daemon
 
 check_verbose_connect () {