Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-02-08 Thread Илья Шипицин
please feel free to either apply or discard patch.

сб, 8 февр. 2025 г. в 07:15, Willy Tarreau :

> Hi Ilya,
>
> On Tue, Feb 04, 2025 at 10:50:24PM +0100,  ??? wrote:
> > well, there are ~10 unhandled "malloc" and ~3 unhandled "calloc" in 51d.c
> > I'm against reviewing/fixing them altogether
> >
> > can we fix it step by step instead ?
>
> If you really need to fix them step by step, that can be OK, but you
> just need to keep in mind that changing a function's construct to
> handle bugs in a way that leaves bugs is always a pain to review and
> even debug later. I think that there might be some cases where bugs
> are sufficiently unrelated to be fixed separately, but for example
> when you need to revisit the unrolling of a function's free() calls
> at the end, you often need to do it all at once in the function, and
> in this case I'd rather not try to reintroduce existing bugs for the
> purpose of sticking to step-by-step, but rather commit a single
> "revisit error paths in function foo() to properly release all
> allocations".
>
> > as for missing space, I can fix (or this can be fixed when applying)
>
> OK noted, thanks!
> Willy
>


Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-02-07 Thread Willy Tarreau
Hi Ilya,

On Tue, Feb 04, 2025 at 10:50:24PM +0100,  ??? wrote:
> well, there are ~10 unhandled "malloc" and ~3 unhandled "calloc" in 51d.c
> I'm against reviewing/fixing them altogether
> 
> can we fix it step by step instead ?

If you really need to fix them step by step, that can be OK, but you
just need to keep in mind that changing a function's construct to
handle bugs in a way that leaves bugs is always a pain to review and
even debug later. I think that there might be some cases where bugs
are sufficiently unrelated to be fixed separately, but for example
when you need to revisit the unrolling of a function's free() calls
at the end, you often need to do it all at once in the function, and
in this case I'd rather not try to reintroduce existing bugs for the
purpose of sticking to step-by-step, but rather commit a single
"revisit error paths in function foo() to properly release all
allocations".

> as for missing space, I can fix (or this can be fixed when applying)

OK noted, thanks!
Willy




Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-02-04 Thread Илья Шипицин
well, there are ~10 unhandled "malloc" and ~3 unhandled "calloc" in 51d.c
I'm against reviewing/fixing them altogether

can we fix it step by step instead ?

as for missing space, I can fix (or this can be fixed when applying)

пт, 3 янв. 2025 г. в 05:14, Willy Tarreau :

> Hi Ilya,
>
> On Thu, Jan 02, 2025 at 10:02:01PM +0100,  ??? wrote:
> > ??, 2 ???. 2025 ?. ? 21:46, Miroslav Zagorac :
> >
> > > On 02. 01. 2025. 21:40,  ??? wrote:
> > > > Honestly, I think those elements must be deallocated on program exit,
> > > > not only if something failed during allocation.
> > > >
> > > > but I did not check that
> > > >
> > >
> > > That is correct.  However, the calloc() result is not checked before
> > > strdup()
> > > either, so the patch is not good.
> > >
> >
> > I did not pretend to add "calloc" check for this patch.
> > we have dedicated *dev/coccinelle/unchecked-calloc.cocci *script which
> > allows us to detect unchecked "calloc". no worry, it won't be forgotten
>
> In general, when reworking some functions' memory allocation and checks,
> it's better to fix it at once when you find that multiple checks are
> missing, rather than attempting incremental fixes that remain partially
> incorrect.
>
> One reason is that very often, dealing with allocations unrolling requires
> an exit label where allocations are unrolled in reverse order, and doing
> them one at a time tends to stay away from that approach. Or sometimes
> you'll figure that fixing certain unchecked allocations require to
> completely change the approach that was used for previous fixes.
>
> Thus if you think you've figured how to completely fix that function, do
> not hesitate, please just fix it all at once, indicating in the commit
> message what you fixed. If you think you can fix it incrementatlly without
> having to change your fix later, then it's fine to do it that way as well
> of course.
>
> > > >>> + if (name->name == NULL) {
> > > >>> + memprintf(err,"Out of memory.");
>   
> BTW, beware of the missing space here.
>
> > > >>> + goto fail_free_name;
> > > >>> + }
>
> Cheers,
> Willy
>


Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-01-02 Thread Willy Tarreau
Hi Ilya,

On Thu, Jan 02, 2025 at 10:02:01PM +0100,  ??? wrote:
> ??, 2 ???. 2025 ?. ? 21:46, Miroslav Zagorac :
> 
> > On 02. 01. 2025. 21:40,  ??? wrote:
> > > Honestly, I think those elements must be deallocated on program exit,
> > > not only if something failed during allocation.
> > >
> > > but I did not check that
> > >
> >
> > That is correct.  However, the calloc() result is not checked before
> > strdup()
> > either, so the patch is not good.
> >
> 
> I did not pretend to add "calloc" check for this patch.
> we have dedicated *dev/coccinelle/unchecked-calloc.cocci *script which
> allows us to detect unchecked "calloc". no worry, it won't be forgotten

In general, when reworking some functions' memory allocation and checks,
it's better to fix it at once when you find that multiple checks are
missing, rather than attempting incremental fixes that remain partially
incorrect.

One reason is that very often, dealing with allocations unrolling requires
an exit label where allocations are unrolled in reverse order, and doing
them one at a time tends to stay away from that approach. Or sometimes
you'll figure that fixing certain unchecked allocations require to
completely change the approach that was used for previous fixes.

Thus if you think you've figured how to completely fix that function, do
not hesitate, please just fix it all at once, indicating in the commit
message what you fixed. If you think you can fix it incrementatlly without
having to change your fix later, then it's fine to do it that way as well
of course.

> > >>> + if (name->name == NULL) {
> > >>> + memprintf(err,"Out of memory.");
  
BTW, beware of the missing space here.

> > >>> + goto fail_free_name;
> > >>> + }

Cheers,
Willy




Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-01-02 Thread Miroslav Zagorac
On 02. 01. 2025. 22:02, Илья Шипицин wrote:
> I did not pretend to add "calloc" check for this patch.
> we have dedicated *dev/coccinelle/unchecked-calloc.cocci *script which
> allows us to detect unchecked "calloc". no worry, it won't be forgotten
> 

OK then;  thank you for your help.

-- 
Miroslav Zagorac

What can change the nature of a man?




Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-01-02 Thread Илья Шипицин
чт, 2 янв. 2025 г. в 21:46, Miroslav Zagorac :

> On 02. 01. 2025. 21:40, Илья Шипицин wrote:
> > Honestly, I think those elements must be deallocated on program exit,
> > not only if something failed during allocation.
> >
> > but I did not check that
> >
>
> That is correct.  However, the calloc() result is not checked before
> strdup()
> either, so the patch is not good.
>

I did not pretend to add "calloc" check for this patch.
we have dedicated *dev/coccinelle/unchecked-calloc.cocci *script which
allows us to detect unchecked "calloc". no worry, it won't be forgotten



>
> >>>   while (*(args[cur_arg])) {
> >>>   name = calloc(1, sizeof(*name));
> >>>   name->name = strdup(args[cur_arg]);
> >>> + if (name->name == NULL) {
> >>> + memprintf(err,"Out of memory.");
> >>> + goto fail_free_name;
> >>> + }
> >>>   LIST_APPEND(&global_51degrees.property_names,
> &name->list);
> >>>   ++cur_arg;
> >>>   }
> >>>
> >>>   return 0;
> >>> +
> >>> +fail_free_name:
> >>> + free(name);
> >>> +fail:
> >>> + return -1;
>
> --
> Miroslav Zagorac
>
> What can change the nature of a man?
>


Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-01-02 Thread Miroslav Zagorac
On 02. 01. 2025. 21:40, Илья Шипицин wrote:
> Honestly, I think those elements must be deallocated on program exit,
> not only if something failed during allocation.
> 
> but I did not check that
> 

That is correct.  However, the calloc() result is not checked before strdup()
either, so the patch is not good.

>>>   while (*(args[cur_arg])) {
>>>   name = calloc(1, sizeof(*name));
>>>   name->name = strdup(args[cur_arg]);
>>> + if (name->name == NULL) {
>>> + memprintf(err,"Out of memory.");
>>> + goto fail_free_name;
>>> + }
>>>   LIST_APPEND(&global_51degrees.property_names, &name->list);
>>>   ++cur_arg;
>>>   }
>>>
>>>   return 0;
>>> +
>>> +fail_free_name:
>>> + free(name);
>>> +fail:
>>> + return -1;

-- 
Miroslav Zagorac

What can change the nature of a man?




Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-01-02 Thread Илья Шипицин
Honestly, I think those elements must be deallocated on program exit,
not only if something failed during allocation.

but I did not check that

чт, 2 янв. 2025 г. в 20:13, Miroslav Zagorac :

> On 02. 01. 2025. 15:18, Ilia Shipitsin wrote:
> > This defect was found by the coccinelle script "unchecked-strdup.cocci".
> > It can be backported to all supported branches.
>
> Hello Ilia,
>
> due to allocating memory for list elements, in case of impossibility of
> memory
> allocation, the previously added list elements should be removed and the
> memory occupied by them should be deallocated.
>
> Because of that I think the second part of the patch is not good enough and
> I'm not sure that it contributes to the quality of the code.
>
> Of course, the code checks if the memory is allocated, so it might prevent
> a
> segmentation fault at some point, but it won't prevent a memory leak.
>
> >   while (*(args[cur_arg])) {
> >   name = calloc(1, sizeof(*name));
> >   name->name = strdup(args[cur_arg]);
> > + if (name->name == NULL) {
> > + memprintf(err,"Out of memory.");
> > + goto fail_free_name;
> > + }
> >   LIST_APPEND(&global_51degrees.property_names, &name->list);
> >   ++cur_arg;
> >   }
> >
> >   return 0;
> > +
> > +fail_free_name:
> > + free(name);
> > +fail:
> > + return -1;
>
>
> Best regards,
>
> --
> Miroslav Zagorac
>
> What can change the nature of a man?
>


Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-01-02 Thread Miroslav Zagorac
On 02. 01. 2025. 15:18, Ilia Shipitsin wrote:
> This defect was found by the coccinelle script "unchecked-strdup.cocci".
> It can be backported to all supported branches.

Hello Ilia,

due to allocating memory for list elements, in case of impossibility of memory
allocation, the previously added list elements should be removed and the
memory occupied by them should be deallocated.

Because of that I think the second part of the patch is not good enough and
I'm not sure that it contributes to the quality of the code.

Of course, the code checks if the memory is allocated, so it might prevent a
segmentation fault at some point, but it won't prevent a memory leak.

>   while (*(args[cur_arg])) {
>   name = calloc(1, sizeof(*name));
>   name->name = strdup(args[cur_arg]);
> + if (name->name == NULL) {
> + memprintf(err,"Out of memory.");
> + goto fail_free_name;
> + }
>   LIST_APPEND(&global_51degrees.property_names, &name->list);
>   ++cur_arg;
>   }
>  
>   return 0;
> +
> +fail_free_name:
> + free(name);
> +fail:
> + return -1;


Best regards,

-- 
Miroslav Zagorac

What can change the nature of a man?




[PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure

2025-01-02 Thread Ilia Shipitsin
This defect was found by the coccinelle script "unchecked-strdup.cocci".
It can be backported to all supported branches.
---
 addons/51degrees/51d.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/addons/51degrees/51d.c b/addons/51degrees/51d.c
index a23b468d6..4f0f9f88f 100644
--- a/addons/51degrees/51d.c
+++ b/addons/51degrees/51d.c
@@ -107,6 +107,10 @@ static int _51d_data_file(char **args, int section_type, 
struct proxy *curpx,
if (global_51degrees.data_file_path)
free(global_51degrees.data_file_path);
global_51degrees.data_file_path = strdup(args[1]);
+   if (global_51degrees.data_file_path == NULL) {
+   memprintf(err,"Out of memory.");
+   return -1;
+   }
 
return 0;
 }
@@ -122,17 +126,26 @@ static int _51d_property_name_list(char **args, int 
section_type, struct proxy *
memprintf(err,
  "'%s' expects at least one 51Degrees property name.",
  args[0]);
-   return -1;
+   goto fail;
}
 
while (*(args[cur_arg])) {
name = calloc(1, sizeof(*name));
name->name = strdup(args[cur_arg]);
+   if (name->name == NULL) {
+   memprintf(err,"Out of memory.");
+   goto fail_free_name;
+   }
LIST_APPEND(&global_51degrees.property_names, &name->list);
++cur_arg;
}
 
return 0;
+
+fail_free_name:
+   free(name);
+fail:
+   return -1;
 }
 
 static int _51d_property_separator(char **args, int section_type, struct proxy 
*curpx,
-- 
2.46.0.windows.1