[Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
G'day,
Since a release of IM is imminent, I thought I'd re-submit a set of
changes that generated a large number of diagnostics from GCC, and
for which the changes were trivial to review, simplified the code,
and are in strict conformance with C's definition of how free(3) is
mandated to work when its parameter is NULL (codified in C89 standard,
and included in all C and C++ standards since).
The patch is in the file "im-trunk___src-process-im_analyze.patch".
The patch is based on standing at the root (trunk) of the IM
Subversion repository, editing just src/process/im_analyze.cpp.
Comparison of the output of "parse-build.lua" before and after
the patch is included in the files "im-r788-1.out" and
"im-r788-2.out": The patch drops the number of warning diagnostic
lines (found by the script) from 500 to 438 lines.
--
More simple changes could be made:
* Remove "Variable declared but not used" code; and
* Remove "Variable set but not used" code;
but these are spread across multiple source files, and so are
arguably more disruptive, with IM now close to a release deadline.
You can see the reports in either of the "parse-build.lua" output
files.
--
cheers,
sur-behoffski (Brenton Hoff)
programmer, Grouse Software
Index: im/src/process/im_analyze.cpp
===
--- im/src/process/im_analyze.cpp (revision 788)
+++ im/src/process/im_analyze.cpp (working copy)
@@ -548,8 +548,7 @@
if (data_cy) data_cy[i] /= (double)data_area[i];
}
- if (local_data_area)
-free(local_data_area);
+ free(local_data_area);
imProcessCounterEnd(counter);
return processing;
@@ -704,8 +703,8 @@
if (!ret)
{
- if (local_data_cx) free(local_data_cx);
- if (local_data_cy) free(local_data_cy);
+ free(local_data_cx);
+ free(local_data_cy);
imProcessCounterEnd(counter);
return 0;
}
@@ -721,10 +720,10 @@
ret = iCalcMoment(cm20, 2, 0, image, data_cx, data_cy, region_count, counter);
if (!ret)
{
-if (local_data_area) free(local_data_area);
-if (local_data_cx) free(local_data_cx);
-if (local_data_cy) free(local_data_cy);
-if (cm20) free(cm20); if (cm02) free(cm02); if (cm11) free(cm11);
+free(local_data_area);
+free(local_data_cx);
+free(local_data_cy);
+free(cm20); free(cm02); free(cm11);
imProcessCounterEnd(counter);
return 0;
}
@@ -731,10 +730,10 @@
ret = iCalcMoment(cm02, 0, 2, image, data_cx, data_cy, region_count, counter);
if (!ret)
{
-if (local_data_area) free(local_data_area);
-if (local_data_cx) free(local_data_cx);
-if (local_data_cy) free(local_data_cy);
-if (cm20) free(cm20); if (cm02) free(cm02); if (cm11) free(cm11);
+free(local_data_area);
+free(local_data_cx);
+free(local_data_cy);
+free(cm20); free(cm02); free(cm11);
imProcessCounterEnd(counter);
return 0;
}
@@ -741,10 +740,10 @@
ret = iCalcMoment(cm11, 1, 1, image, data_cx, data_cy, region_count, counter);
if (!ret)
{
-if (local_data_area) free(local_data_area);
-if (local_data_cx) free(local_data_cx);
-if (local_data_cy) free(local_data_cy);
-if (cm20) free(cm20); if (cm02) free(cm02); if (cm11) free(cm11);
+free(local_data_area);
+free(local_data_cx);
+free(local_data_cy);
+free(cm20); free(cm02); free(cm11);
imProcessCounterEnd(counter);
return 0;
}
@@ -827,17 +826,17 @@
if (!imCounterInc(counter))
{
- if (local_major_slope) free(local_major_slope);
- if (local_minor_slope) free(local_minor_slope);
+ free(local_major_slope);
+ free(local_minor_slope);
free(A1);
free(A2);
free(C1);
free(C2);
- if (local_data_area) free(local_data_area);
- if (local_data_cx) free(local_data_cx);
- if (local_data_cy) free(local_data_cy);
- if (cm20) free(cm20); if (cm02) free(cm02); if (cm11) free(cm11);
+ free(local_data_area);
+ free(local_data_cx);
+ free(local_data_cy);
+ free(cm20); free(cm02); free(cm11);
imProcessCounterEnd(counter);
return 0;
@@ -956,17 +955,17 @@
free(D1a);
free(D2a);
- if (local_major_slope) free(local_major_slope);
- if (local_minor_slope) free(local_minor_slope);
+ free(local_major_slope);
+ free(local_minor_slope);
free(A1);
free(A2);
free(C1);
free(C2);
- if (local_data_area) free(local_data_area);
- if (local_data_cx) free(local_data_cx);
- if (local_data_cy) free(local_data_cy);
- if (cm20) free(cm20); if (cm02) free(cm02); if (cm11) free(cm11);
+ free(local_data_area);
+ free(local_data_cx);
+ free(local_data_cy);
+ free(cm20); free(cm02); free(cm11);
imProcessCounterEnd(counter);
return ret;
@@ -1106,7 +1105,7 @@
}
}
- if (holes_perim) free(holes_perim);
+ free(holes_perim);
free(holes_area);
imImageDestroy(holes_image);
* No diagnostics for:
#include expects "FILENA
Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
As already mentioned, most of the report refers to the third-party library, where they do not have access. Could you forward this report to the maintainers of these libraries? regards, Ranier Vilela ___ Iup-users mailing list Iup-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/iup-users
Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
On May 17 2020, sur-behoffski wrote: and are in strict conformance with C's definition of how free(3) is mandated to work when its parameter is NULL (codified in C89 standard, and included in all C and C++ standards since). IMHO it's still better to NOT call free(3) if there is no memory to be released. Let alone changing good code in order to rely on free accepting a NULL pointer too. ___ Iup-users mailing list Iup-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/iup-users
Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
For me those "if"s are there also to remind me that those pointers where not allocated. For me one call to malloc/calloc must match one call to free. Maybe I'm old, but I'll stick with that for now. What I can do is to improve the indentation and readability. Also cm20, cm02 and cm11 don't need the if. Just committed to the SVN. Best, Scuri Em dom, 17 de mai de 2020 10:20, Jörg F. Wittenberger < joerg.wittenber...@softeyes.net> escreveu: > On May 17 2020, sur-behoffski wrote: > > >and are in strict conformance with C's definition of how free(3) is > >mandated to work when its parameter is NULL (codified in C89 standard, > >and included in all C and C++ standards since). > > IMHO it's still better to NOT call free(3) if there is no memory to be > released. Let alone changing good code in order to rely on free accepting > a > NULL pointer too. > > > > ___ > Iup-users mailing list > Iup-users@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/iup-users > ___ Iup-users mailing list Iup-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/iup-users
Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
De: Antonio Scuri
Enviado: domingo, 17 de maio de 2020 17:23
Para: IUP discussion list.
Assunto: Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow
free(NULL); " patch...
> For me those "if"s are there also to remind me that those pointers where not
> allocated. For me one call to >malloc/calloc must match one call to free.
> Maybe I'm old, but I'll stick with that for now.
Certainly "call to malloc/calloc must match one call to free" is that is
correct, even if, two free for the same malloc would be double free.
The point is that IF it protects you from double free, only if the pointer is
marked as NULL, after the free as:
if (ptr) {
free(ptr);
ptr = NULL;
}
This is certainly defensive programming and protects the user, but it hides the
fact that they have a double free bug in the program.
At runtime, it makes no difference, calling free with NULL, as long as the
pointer has not been released before.
ptr = NULL;
free(ptr);
The big question is how to catch the double free that may exist.
Because this not to protect agaist double free.
if (ptr) {
free(ptr);
}
regards,
Ranier Vilela
___
Iup-users mailing list
Iup-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iup-users
Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
De: Antonio Scuri
Enviado: domingo, 17 de maio de 2020 17:23
Para: IUP discussion list.
Assunto: Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow
free(NULL); " patch...
> For me those "if"s are there also to remind me that those pointers where not
> allocated. For me one call to >malloc/calloc must match one call to free.
> Maybe I'm old, but I'll stick with that for now.
Certainly "call to malloc/calloc must match one call to free" is that is
correct, even if, two free for the same malloc would be double free.
The point is that IF it protects you from double free, only if the pointer is
marked as NULL, after the free as:
if (ptr) {
free(ptr);
ptr = NULL;
}
This is certainly defensive programming and protects the user, but it hides the
fact that they have a double free bug in the program.
At runtime, it makes no difference, calling free with NULL, as long as the
pointer has not been released before.
ptr = NULL;
free(ptr);
The big question is how to catch the double free that may exist.
Because this not to protect agaist double free.
if (ptr) {
free(ptr);
}
regards,
Ranier Vilela
___
Iup-users mailing list
Iup-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iup-users
Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
On Sunday, 17 May 2020, 23:05:05 BST, Ranier Vilela
wrote:
The big question is how to catch the double free that may exist.
Because this not to protect against double free.
if (ptr) {
free(ptr);
}
One tactic I have just recently started adopting, entirely independent of this
and in fact in a completely different language is:
ptr = ffree(ptr)
where ffree() always returns null.
___
Iup-users mailing list
Iup-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iup-users
Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow free(NULL); " patch...
De: Pete Lomax via Iup-users
Enviado: segunda-feira, 18 de maio de 2020 02:34
Para: IUP discussion list.
Cc: Pete Lomax
Assunto: Re: [Iup-users] Repeat of IM's "process/im_analyze.cpp allow
free(NULL); " patch...
On Sunday, 17 May 2020, 23:05:05 BST, Ranier Vilela
wrote:
The big question is how to catch the double free that may exist.
Because this not to protect against double free.
if (ptr) {
free(ptr);
}
>One tactic I have just recently started adopting, entirely independent of this
>and in fact in a completely different l>anguage is:
>ptr = ffree(ptr)
>where ffree() always returns null.
FFree comes in the case of defensive programming and depending on what is
inside it, it remains a waste.
Probably ffree is written something like this:
void * ffree (void * ptr) {
if (ptr) {
free (ptr)
}
return NULL;
}
I am using only free, without any ifs, and using tools to detect memory
problems, such as DrMemory on Windows.
If a double free occurs, it will certainly be detected either by me or by the
user.
regards,
Ranier Vilela
___
Iup-users mailing list
Iup-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iup-users
