Re: [PATCH v5 1/3] daemon: replace atoi() with strtoul_ui() and strtol_i()

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