| Message ID | d9c997d7a9c8975ce845aa0cb4deaba22cbf3b94.1729669221.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | parse: replace atoi() with strtoul_ui() and strtol_i() | expand |
On Wed, Oct 23, 2024 at 07:40:18AM +0000, Usman Akinyemi via GitGitGadget wrote: > 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 | 12 ++++++++---- > t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/daemon.c b/daemon.c > index cb946e3c95f..a40e435c637 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -4,6 +4,7 @@ > #include "abspath.h" > #include "config.h" > #include "environment.h" > +#include "gettext.h" > #include "path.h" > #include "pkt-line.h" > #include "protocol.h" > @@ -1308,17 +1309,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 && Hmmph. test_write_lines is typically called like test_write_lines 1 2 3 to write a file which contains three lines where each of the arguments appears on its own line. But here you pass a single argument, which causes us to write out a single line. Is there a reason for this? If not, I would expect us to write: echo "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expect Or, perhaps even cleaner would be to do: test_must_fail git daemon --init-timeout="$arg" 2>err && test_grep "invalid init-timeout ${SQ}$arg${SQ}" err since I don't think asserting on the actual error contents matching verbatim what we expect is adding all that much. (Also throughout you write 2>actual_err, but redirecting 2>err is more concise and in convention with the rest of the test suite's style). > + 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 > +' > + Same notes from above in these two as well. Thanks, Taylor
On Wed, Oct 23, 2024 at 8:31 PM Taylor Blau <me@ttaylorr.com> wrote: > > On Wed, Oct 23, 2024 at 07:40:18AM +0000, Usman Akinyemi via GitGitGadget wrote: > > 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 | 12 ++++++++---- > > t/t5570-git-daemon.sh | 26 ++++++++++++++++++++++++++ > > 2 files changed, 34 insertions(+), 4 deletions(-) > > > > diff --git a/daemon.c b/daemon.c > > index cb946e3c95f..a40e435c637 100644 > > --- a/daemon.c > > +++ b/daemon.c > > @@ -4,6 +4,7 @@ > > #include "abspath.h" > > #include "config.h" > > #include "environment.h" > > +#include "gettext.h" > > #include "path.h" > > #include "pkt-line.h" > > #include "protocol.h" > > @@ -1308,17 +1309,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 && > > Hmmph. test_write_lines is typically called like > > test_write_lines 1 2 3 > > to write a file which contains three lines where each of the arguments > appears on its own line. > > But here you pass a single argument, which causes us to write out a > single line. Is there a reason for this? If not, I would expect us to > write: > > echo "fatal: invalid init-timeout ${SQ}$arg${SQ}, expecting a non-negative integer" >expect > > Or, perhaps even cleaner would be to do: > > test_must_fail git daemon --init-timeout="$arg" 2>err && > test_grep "invalid init-timeout ${SQ}$arg${SQ}" err > Thanks and noted, I will use this approach. I will send a new patch to fix this. Usman > since I don't think asserting on the actual error contents matching > verbatim what we expect is adding all that much. > > (Also throughout you write 2>actual_err, but redirecting 2>err is more > concise and in convention with the rest of the test suite's style). > > > + 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 > > +' > > + > > Same notes from above in these two as well. > > Thanks, > Taylor
diff --git a/daemon.c b/daemon.c index cb946e3c95f..a40e435c637 100644 --- a/daemon.c +++ b/daemon.c @@ -4,6 +4,7 @@ #include "abspath.h" #include "config.h" #include "environment.h" +#include "gettext.h" #include "path.h" #include "pkt-line.h" #include "protocol.h" @@ -1308,17 +1309,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 () {