Re: [PATCH 1/3] BUG/MINOR: namespace: handle a possible strdup() failure

2024-12-09 Thread Willy Tarreau
On Tue, Dec 03, 2024 at 08:20:16PM +0100,  ??? wrote:
> thank for suggestions.
> here's v2 of patchset

Looks good, all applied now, thank you Ilya!
Willy




Re: [PATCH 1/3] BUG/MINOR: namespace: handle a possible strdup() failure

2024-12-03 Thread Илья Шипицин
thank for suggestions.
here's v2 of patchset

сб, 30 нояб. 2024 г. в 13:33, Willy Tarreau :

> Hi Ilya,
>
> On Thu, Nov 28, 2024 at 01:07:40AM +0100, Ilya Shipitsin wrote:
> > This defect was found by the coccinelle script "unchecked-strdup.cocci".
> > It can be backported to all supported branches.
> > ---
> >  src/namespace.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/src/namespace.c b/src/namespace.c
> > index 9cc85a384..12885cd9f 100644
> > --- a/src/namespace.c
> > +++ b/src/namespace.c
> > @@ -92,6 +92,11 @@ struct netns_entry* netns_store_insert(const char
> *ns_name)
> >   goto out;
> >   entry->fd = fd;
> >   entry->node.key = strdup(ns_name);
> > + if (!entry->node.key) {
> > + free(entry);
> > + entry = NULL;
>
> Above, you can use ha_free(&entry), which performs both the free() and
> sets the NULL. We try to use it more so that missing NULLs are easier to
> spot (i.e. only free() is suspicious and needs longer investigation).
>
> Also, the previously created fd will leak, so a close(fd) is missing.
>
> Thanks!
> Willy
>
From 95bf14da420a7ec7c5097e28b564f6bf33bc3884 Mon Sep 17 00:00:00 2001
From: Ilia Shipitsin 
Date: Tue, 3 Dec 2024 17:10:21 +0100
Subject: [PATCH 1/3] BUG/MINOR: namespace: handle a possible strdup() failure

This defect was found by the coccinelle script "unchecked-strdup.cocci".
It can be backported to all supported branches.
---
 src/namespace.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/namespace.c b/src/namespace.c
index 9cc85a384..38464bd4c 100644
--- a/src/namespace.c
+++ b/src/namespace.c
@@ -89,13 +89,23 @@ struct netns_entry* netns_store_insert(const char *ns_name)
 
entry = calloc(1, sizeof(*entry));
if (!entry)
-   goto out;
+   goto err_close_fd;
entry->fd = fd;
entry->node.key = strdup(ns_name);
+   if (!entry->node.key)
+   goto err_free_entry;
+
entry->name_len = strlen(ns_name);
ebis_insert(&namespace_tree_root, &entry->node);
 out:
return entry;
+
+/* free all allocated stuff and return entry */
+err_free_entry:
+   ha_free(&entry);
+err_close_fd:
+   close(fd);
+   return entry;
 }
 
 const struct netns_entry* netns_store_lookup(const char *ns_name, size_t 
ns_name_len)
-- 
2.46.0.windows.1

From 7a71bb88a8812ab8ffa9d522ac71cb79f04cc73b Mon Sep 17 00:00:00 2001
From: Ilia Shipitsin 
Date: Tue, 3 Dec 2024 19:54:46 +0100
Subject: [PATCH 3/3] BUG/MINOR: resolvers: handle a possible strdup() failure

This defect was found by the coccinelle script "unchecked-strdup.cocci".
It can be backported to all supported branches.
---
 src/resolvers.c | 28 +++-
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/resolvers.c b/src/resolvers.c
index f8f0c8edf..e1550cf12 100644
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -3489,13 +3489,17 @@ static int resolvers_new(struct resolvers **resolvers, 
const char *id, const cha
p = calloc(1, sizeof *p);
if (!p) {
err_code |= ERR_ALERT | ERR_FATAL;
-   goto out;
+   goto err_free_r;
}
 
init_new_proxy(p);
resolvers_setup_proxy(p);
p->parent = r;
p->id = strdup(id);
+   if (!p->id) {
+   err_code |= ERR_ALERT | ERR_FATAL;
+   goto err_free_p;
+   }
p->conf.args.file = p->conf.file = copy_file_name(file);
p->conf.args.line = p->conf.line = linenum;
r->px = p;
@@ -3503,8 +3507,16 @@ static int resolvers_new(struct resolvers **resolvers, 
const char *id, const cha
/* default values */
LIST_APPEND(&sec_resolvers, &r->list);
r->conf.file = strdup(file);
+   if (!r->conf.file) {
+   err_code |= ERR_ALERT | ERR_FATAL;
+   goto err_free_p_id;
+   }
r->conf.line = linenum;
r->id = strdup(id);
+   if (!r->id) {
+   err_code |= ERR_ALERT | ERR_FATAL;
+   goto err_free_conf_file;
+   }
r->query_ids = EB_ROOT;
/* default maximum response size */
r->accepted_payload_size = 512;
@@ -3528,11 +3540,17 @@ static int resolvers_new(struct resolvers **resolvers, 
const char *id, const cha
*resolvers = r;
 
 out:
-   if (err_code & (ERR_FATAL|ERR_ABORT)) {
-   ha_free(&r);
-   ha_free(&p);
-   }
+   return err_code;
 
+/* free all allocated stuff and return err_code */
+err_free_conf_file:
+   ha_free((void **)&r->conf.file);
+err_free_p_id:
+   ha_free(&p->id);
+err_free_p:
+   ha_free(&p);
+err_free_r:
+   ha_free(&r);
return err_code;
 }
 
-- 
2.46.0.windows.1

From 520dc81b6b42a7cfd008d668d4851c4858453e32 Mon Sep 17 00:00:00 2001
From: Ilia Shipitsin 
Date: Tue, 3 Dec 2024 17:13:05 +0100
Subject: [PATCH 2/3] BUG/MINOR: ssl_crtlist: handle a possible st

Re: [PATCH 1/3] BUG/MINOR: namespace: handle a possible strdup() failure

2024-11-30 Thread Willy Tarreau
Hi Ilya,

On Thu, Nov 28, 2024 at 01:07:40AM +0100, Ilya Shipitsin wrote:
> This defect was found by the coccinelle script "unchecked-strdup.cocci".
> It can be backported to all supported branches.
> ---
>  src/namespace.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/namespace.c b/src/namespace.c
> index 9cc85a384..12885cd9f 100644
> --- a/src/namespace.c
> +++ b/src/namespace.c
> @@ -92,6 +92,11 @@ struct netns_entry* netns_store_insert(const char *ns_name)
>   goto out;
>   entry->fd = fd;
>   entry->node.key = strdup(ns_name);
> + if (!entry->node.key) {
> + free(entry);
> + entry = NULL;

Above, you can use ha_free(&entry), which performs both the free() and
sets the NULL. We try to use it more so that missing NULLs are easier to
spot (i.e. only free() is suspicious and needs longer investigation).

Also, the previously created fd will leak, so a close(fd) is missing.

Thanks!
Willy