On Mon, Aug 05, 2024 at 09:02:46PM +0200, ???? ??????? wrote:
> updated patch attached (I preferred that instead of sending with "git
> send-email")
>
> ??, 5 ???. 2024 ?. ? 20:10, ???? ??????? <chipits...@gmail.com>:
>
> >
> >
> > ??, 5 ???. 2024 ?. ? 20:09, William Lallemand <wlallem...@irq6.net>:
> >
> >> On Mon, Aug 05, 2024 at 08:01:39PM +0200, ???? ??????? wrote:
> >> > Subject: Re: [PATCH] src/fcgi-app.c: handle strdup failure
> >> > ??, 5 ???. 2024 ?. ? 19:56, William Lallemand <wlallem...@irq6.net>:
> >> >
> >> > > On Mon, Aug 05, 2024 at 07:17:48PM +0200, Ilia Shipitsin wrote:
> >> > > > Subject: [PATCH] src/fcgi-app.c: handle strdup failure
> >> > > > found by coccinelle
> >> > >
> >> > > Please add clearer commit messages in your patches, you tend to
> >> minimize
> >> > > them, thanks ! :-)
> >> > >
> >> >
> >> > truth to be told, I tend to write longer messages than usually :)
> >> >
> >> >
> >> > >
> >> > > > ---
> >> > > > src/fcgi-app.c | 5 ++++-
> >> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> >> > > >
> >> > > > diff --git a/src/fcgi-app.c b/src/fcgi-app.c
> >> > > > index b3a9b7c59..d96bb222c 100644
> >> > > > --- a/src/fcgi-app.c
> >> > > > +++ b/src/fcgi-app.c
> >> > > > @@ -606,6 +606,8 @@ static int proxy_parse_use_fcgi_app(char
> >> **args, int
> >> > > section, struct proxy *curp
> >> > > > if (!fcgi_conf)
> >> > > > goto err;
> >> > > > fcgi_conf->name = strdup(args[1]);
> >> > > > + if (!fcgi_conf->name)
> >> > > > + goto err;
> >> > > > LIST_INIT(&fcgi_conf->param_rules);
> >> > > > LIST_INIT(&fcgi_conf->hdr_rules);
> >> > > >
> >> > > > @@ -622,7 +624,8 @@ static int proxy_parse_use_fcgi_app(char
> >> **args, int
> >> > > section, struct proxy *curp
> >> > > > return retval;
> >> > > > err:
> >> > > > if (fcgi_conf) {
> >> > > > - free(fcgi_conf->name);
> >> > > > + if (fcgi_conf->name)
> >> > > > + free(fcgi_conf->name);
> >> > >
> >> > > You don't need to add a check there, free(NULL) does nothing.
> >> > >
> >> >
> >> > I tried to figure out whether that behaviour is by chance or by purpose
> >> > (taking into account variety of implementations on different OSes)
> >> >
> >> > I'll try again
> >> >
> >>
> >> This is a standard behavior that is specified in POSIX, honestly HAProxy
> >> won't probably run if it wasn't behaving this
> >> way on some weird OS.
> >>
> >> https://pubs.opengroup.org/onlinepubs/009695399/functions/free.html "If
> >> ptr is a null pointer, no action shall occur."
> >>
> >
> > "man 3 free" didn't give me any link to POSIX.
> > I've should have a look at POSIX directly, but I didn't
> >
> >
> >>
> >> --
> >> William Lallemand
> >>
> >
> From baa55f1434b2ed5288536f62b8e322ed20904b8d Mon Sep 17 00:00:00 2001
> From: Ilia Shipitsin <chipits...@gmail.com>
> Date: Mon, 5 Aug 2024 20:59:09 +0200
> Subject: [PATCH] src/fcgi-app.c: handle strdup failure
>
> this defect is found by the following coccinelle script:
>
> // find calls to strdup
> @call@
> expression ptr;
> position p;
> @@
>
> ptr@p = strdup(...);
>
> // find ok calls to strdup
> @ok@
> expression ptr;
> position call.p;
> @@
>
> ptr@p = strdup(...);
> ... when != ptr
> (
> (ptr == NULL || ...)
> |
> (ptr == 0 || ...)
> |
> (ptr != NULL || ...)
> |
> (ptr != 0 || ...)
> )
>
> // fix bad calls to strdup
> @depends on !ok@
> expression ptr;
> position call.p;
> @@
>
> ptr@p = strdup(...);
> + if (ptr == NULL) return;
> ---
> src/fcgi-app.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/fcgi-app.c b/src/fcgi-app.c
> index b3a9b7c59..98077b959 100644
> --- a/src/fcgi-app.c
> +++ b/src/fcgi-app.c
> @@ -606,6 +606,8 @@ static int proxy_parse_use_fcgi_app(char **args, int
> section, struct proxy *curp
> if (!fcgi_conf)
> goto err;
> fcgi_conf->name = strdup(args[1]);
> + if (!fcgi_conf->name)
> + goto err;
> LIST_INIT(&fcgi_conf->param_rules);
> LIST_INIT(&fcgi_conf->hdr_rules);
Looks good to me, thanks! I think we can also commit your cocci script
as dev/coccinelle/strdup.cocci. We'll also mark the fix as BUG/MINOR.
Thanks!
Willy
