Re: [PATCH] BUG/MINOR: 51 degree: handle a possible strdup() failure
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
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
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
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
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
чт, 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
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
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
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
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