On Wed, Oct 23, 2024 at 8:31 PM Taylor Blau [off-list ref] wrote:

On Wed, Oct 23, 2024 at 07:40:18AM +0000, Usman Akinyemi via GitGitGadget wrote:

quoted

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

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).

quoted

+             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