Re: [PATCH v5 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()
From: Taylor Blau <hidden>
Date: 2024-10-23 20:31:05
On Wed, Oct 23, 2024 at 07:40:18AM +0000, Usman Akinyemi via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Usman Akinyemi <redacted> 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 <redacted> --- 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