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 |
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
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 --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 () {