Thread: [Gpsbabel-code] clang scan-build report of gpsbabel
Brought to you by:
robertl
From: Bernd Z. <be...@bz...> - 2012-05-17 13:15:29
|
Hi, I've built gpsbabel with clang's scan-build, the current report (which shows 207 bugs) is available at http://devel.recluse.de/scan-build/gpsbabel/ For now I'll rebuild the sources every night and a new report will be generated, assuming there is some interest to fix those bugs :) Cheers, Bernd -- Bernd Zeimetz Debian GNU/Linux Developer http://bzed.de http://www.debian.org GPG Fingerprint: ECA1 E3F2 8E11 2432 D485 DD95 EB36 171A 6FF9 435F |
From: Thorben T. <r0...@co...> - 2012-05-17 14:02:14
|
On Thu, 17 May 2012 14:50:20 +0200 Bernd Zeimetz <be...@bz...> wrote: > Hi, Hello, i'm not even a gpsbabel developer, but i've looked at the report and report my findings... > I've built gpsbabel with clang's scan-build, the current report (which shows 207 > bugs) is available at > > http://devel.recluse.de/scan-build/gpsbabel/ > > For now I'll rebuild the sources every night and a new report will be generated, > assuming there is some interest to fix those bugs :) sorry, but... have you actually read those reports or understand them? i've wasted some of my time looking at a few, and none of them are bugs, most seem either irrelevant, or false positives... false positive: http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-4qnTWr.html it seems your tool does not resolve macros correcly, and assumes the code would try to operate on an empty list...? http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-TxWcEg.html your tool does not realize this code is part of a loop, and the value will be used on the next iteration? irrelevant: http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-B4Cv3H.html that's an excess initialization of a variable used as a counter. it's hardly a bug, and might rather make the code more readable... http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-nnDoke.html the control variable is already initialized, and instead of leaving the initialization empty, it's assigned to itself... makes the code more readable, and will be optimized out anyway. http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-FyAuMY.html another duplicate initialization of a control variable and so on... http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-hLVZn7.html after no less than 30 branches the code might write to an uninitialized pointer... this might indeed cause a crash on some malformed input... if security was a major issue, an extra check might be justified... this one _might_ be valid, the code is too complex to tell right away: http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-ipSdJi.html an uninitialized value might be read in some unlikely condition when the operation has already failed anyway... > Cheers, > Bernd - T. |
From: Bernd Z. <be...@bz...> - 2012-05-17 15:34:00
|
Hi, > i'm not even a gpsbabel developer, but i've looked at the report and > report my findings... > >> I've built gpsbabel with clang's scan-build, the current report (which shows 207 >> bugs) is available at >> >> http://devel.recluse.de/scan-build/gpsbabel/ >> >> For now I'll rebuild the sources every night and a new report will be generated, >> assuming there is some interest to fix those bugs :) > > sorry, but... > have you actually read those reports or understand them? I did not read all of them, but yes I understand them. Also if you have no idea what scan-build is you should learn about it, then you will also learn that it might produce false positives, but in most cases its findings are just right. If you actually think that there is a real false positive (as in: it is not just something you ignore for your own lazyness of writing good code), please report it on the Clang Static Analyzer development mailinglist - see http://clang-analyzer.llvm.org/ I'm commenting on some of the things you are mentioning where scan-build is right on the first look. Don't have the time or the mood to waste my time on replies to unfriendly mails as yours. > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-TxWcEg.html > your tool does not realize this code is part of a loop, > and the value will be used on the next iteration? The first thing that will be done on the next iteration is slen = 0; So the data saved in slen won't be used again. It could even be that slen=0 sould be moved out of the loop (actually removed as it is initialized already) to give the loop some more sense. Obviously my "tool" realized that. If can't know if the first slen=0; in the loop is wrong or if adding something to slen which is not being used again is wrong, but it shows that there is something wrong that needs to be fixed. > irrelevant: > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-B4Cv3H.html > that's an excess initialization of a variable used as a counter. > it's hardly a bug, and might rather make the code more readable... An initialization about 20 lines about the place where exactly the same line comes up again doesn't make things more readable. Yes, it doesn't do anything bad, but it is also just not needed to have it in there twice. One day somebody will add some code in between the two initializations and will have a fun time trying ti figure out why the introduced code doesn't seem to do anything at all. > and so on... Yes and so on. Obviously you are a fan of having dead code in a project. I leave it to you to learn why dead code / dead stores should be removed. > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-hLVZn7.html > after no less than 30 branches the code might write to an uninitialized > pointer... this might indeed cause a crash on some malformed input... > if security was a major issue, an extra check might be justified... Security is *always* a major issue. There are people running gpsbabel on data provided by users on websites, they expect gpsbabel not to choke on malformed input. Or you just want to convert a "track" you've downloaded from the internet to have a look at it - you want to trust your favourite tool that using it on such data is safe. > this one _might_ be valid, the code is too complex to tell right away: > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-ipSdJi.html > an uninitialized value might be read in some unlikely condition > when the operation has already failed anyway... The code is not complex and the error is well explained. recType is not initialized if neither tracks nor waypoints were handled before looking on routes. I don't think that is an unlikely condition, it is a common task to extract only parts of a given data source. Cheers, Bernd -- Bernd Zeimetz Debian GNU/Linux Developer http://bzed.de http://www.debian.org GPG Fingerprint: ECA1 E3F2 8E11 2432 D485 DD95 EB36 171A 6FF9 435F |
From: Thorben T. <r0...@co...> - 2012-05-17 15:58:53
|
Hello, well, you may be right, and it's surely not a bad thing to alert people to potential issues, but also for some part it's a matter of taste to fix all of the minor problems. what i find a little strange is your assumption that people will be willing to devote their time to fixing issues that you just throw at them as an automatically generated report, instead of, say, giving instructions what to fix, or submitting a patch. but then, please just ignore me, i'm not even a developer of gpsbabel, as i said, after spending some time reading your report, i just thought it'd be appropriate to report my opinion on it. it's really something for mr. lipe to decide on, i'll refrain from further comments. - T. On Thu, 17 May 2012 17:33:48 +0200 Bernd Zeimetz <be...@bz...> wrote: > Hi, > > > i'm not even a gpsbabel developer, but i've looked at the report and > > report my findings... > > > >> I've built gpsbabel with clang's scan-build, the current report (which shows 207 > >> bugs) is available at > >> > >> http://devel.recluse.de/scan-build/gpsbabel/ > >> > >> For now I'll rebuild the sources every night and a new report will be generated, > >> assuming there is some interest to fix those bugs :) > > > > sorry, but... > > have you actually read those reports or understand them? > > I did not read all of them, but yes I understand them. Also if you have no idea > what scan-build is you should learn about it, then you will also learn that it > might produce false positives, but in most cases its findings are just right. If > you actually think that there is a real false positive (as in: it is not just > something you ignore for your own lazyness of writing good code), please report > it on the Clang Static Analyzer development mailinglist - see > http://clang-analyzer.llvm.org/ > I'm commenting on some of the things you are mentioning where scan-build is > right on the first look. Don't have the time or the mood to waste my time on > replies to unfriendly mails as yours. > > > > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-TxWcEg.html > > your tool does not realize this code is part of a loop, > > and the value will be used on the next iteration? > > The first thing that will be done on the next iteration is > slen = 0; > So the data saved in slen won't be used again. It could even be that slen=0 > sould be moved out of the loop (actually removed as it is initialized already) > to give the loop some more sense. Obviously my "tool" realized that. If can't > know if the first slen=0; in the loop is wrong or if adding something to slen > which is not being used again is wrong, but it shows that there is something > wrong that needs to be fixed. > > > > irrelevant: > > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-B4Cv3H.html > > that's an excess initialization of a variable used as a counter. > > it's hardly a bug, and might rather make the code more readable... > > An initialization about 20 lines about the place where exactly the same line > comes up again doesn't make things more readable. Yes, it doesn't do anything > bad, but it is also just not needed to have it in there twice. One day somebody > will add some code in between the two initializations and will have a fun time > trying ti figure out why the introduced code doesn't seem to do anything at all. > > > > and so on... > > Yes and so on. Obviously you are a fan of having dead code in a project. I leave > it to you to learn why dead code / dead stores should be removed. > > > > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-hLVZn7.html > > after no less than 30 branches the code might write to an uninitialized > > pointer... this might indeed cause a crash on some malformed input... > > if security was a major issue, an extra check might be justified... > > Security is *always* a major issue. There are people running gpsbabel on data > provided by users on websites, they expect gpsbabel not to choke on malformed > input. Or you just want to convert a "track" you've downloaded from the internet > to have a look at it - you want to trust your favourite tool that using it on > such data is safe. > > > > this one _might_ be valid, the code is too complex to tell right away: > > http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-ipSdJi.html > > an uninitialized value might be read in some unlikely condition > > when the operation has already failed anyway... > > The code is not complex and the error is well explained. recType is not > initialized if neither tracks nor waypoints were handled before looking on > routes. I don't think that is an unlikely condition, it is a common task to > extract only parts of a given data source. > > > Cheers, > > Bernd > > > -- > Bernd Zeimetz Debian GNU/Linux Developer > http://bzed.de http://www.debian.org > GPG Fingerprint: ECA1 E3F2 8E11 2432 D485 DD95 EB36 171A 6FF9 435F > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > Gpsbabel-code mailing list http://www.gpsbabel.org > Gps...@li... > https://lists.sourceforge.net/lists/listinfo/gpsbabel-code |
From: Bernd Z. <be...@bz...> - 2012-05-17 16:55:45
|
Hi, > well, you may be right, > and it's surely not a bad thing to alert people to potential issues, > but also for some part it's a matter of taste to fix all of the > minor problems. > > what i find a little strange is your assumption that people will be > willing to devote their time to fixing issues that you just throw at > them as an automatically generated report, instead of, say, > giving instructions what to fix, or submitting a patch. I don't assume that people will fix these issues automatically - but usually people are happy if somebody at least looks for such defects. If I find the time I'll also send patches, but for somebody not knowing the code it is much more work in a lot of cases than for somebody who knows it already. > but then, please just ignore me, i'm not even a developer of gpsbabel, > as i said, after spending some time reading your report, i just thought > it'd be appropriate to report my opinion on it. Sure, why not. :) Cheers, Bernd > > it's really something for mr. lipe to decide on, > i'll refrain from further comments. > > - T. > > On Thu, 17 May 2012 17:33:48 +0200 Bernd Zeimetz <be...@bz...> wrote: >> Hi, >> >>> i'm not even a gpsbabel developer, but i've looked at the report and >>> report my findings... >>> >>>> I've built gpsbabel with clang's scan-build, the current report (which shows 207 >>>> bugs) is available at >>>> >>>> http://devel.recluse.de/scan-build/gpsbabel/ >>>> >>>> For now I'll rebuild the sources every night and a new report will be generated, >>>> assuming there is some interest to fix those bugs :) >>> >>> sorry, but... >>> have you actually read those reports or understand them? >> >> I did not read all of them, but yes I understand them. Also if you have no idea >> what scan-build is you should learn about it, then you will also learn that it >> might produce false positives, but in most cases its findings are just right. If >> you actually think that there is a real false positive (as in: it is not just >> something you ignore for your own lazyness of writing good code), please report >> it on the Clang Static Analyzer development mailinglist - see >> http://clang-analyzer.llvm.org/ >> I'm commenting on some of the things you are mentioning where scan-build is >> right on the first look. Don't have the time or the mood to waste my time on >> replies to unfriendly mails as yours. >> >> >>> http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-TxWcEg.html >>> your tool does not realize this code is part of a loop, >>> and the value will be used on the next iteration? >> >> The first thing that will be done on the next iteration is >> slen = 0; >> So the data saved in slen won't be used again. It could even be that slen=0 >> sould be moved out of the loop (actually removed as it is initialized already) >> to give the loop some more sense. Obviously my "tool" realized that. If can't >> know if the first slen=0; in the loop is wrong or if adding something to slen >> which is not being used again is wrong, but it shows that there is something >> wrong that needs to be fixed. >> >> >>> irrelevant: >>> http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-B4Cv3H.html >>> that's an excess initialization of a variable used as a counter. >>> it's hardly a bug, and might rather make the code more readable... >> >> An initialization about 20 lines about the place where exactly the same line >> comes up again doesn't make things more readable. Yes, it doesn't do anything >> bad, but it is also just not needed to have it in there twice. One day somebody >> will add some code in between the two initializations and will have a fun time >> trying ti figure out why the introduced code doesn't seem to do anything at all. >> >> >>> and so on... >> >> Yes and so on. Obviously you are a fan of having dead code in a project. I leave >> it to you to learn why dead code / dead stores should be removed. >> >> >>> http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-hLVZn7.html >>> after no less than 30 branches the code might write to an uninitialized >>> pointer... this might indeed cause a crash on some malformed input... >>> if security was a major issue, an extra check might be justified... >> >> Security is *always* a major issue. There are people running gpsbabel on data >> provided by users on websites, they expect gpsbabel not to choke on malformed >> input. Or you just want to convert a "track" you've downloaded from the internet >> to have a look at it - you want to trust your favourite tool that using it on >> such data is safe. >> >> >>> this one _might_ be valid, the code is too complex to tell right away: >>> http://devel.recluse.de/scan-build/gpsbabel/2012-05-17-1/report-ipSdJi.html >>> an uninitialized value might be read in some unlikely condition >>> when the operation has already failed anyway... >> >> The code is not complex and the error is well explained. recType is not >> initialized if neither tracks nor waypoints were handled before looking on >> routes. I don't think that is an unlikely condition, it is a common task to >> extract only parts of a given data source. >> >> >> Cheers, >> >> Bernd >> >> >> -- >> Bernd Zeimetz Debian GNU/Linux Developer >> http://bzed.de http://www.debian.org >> GPG Fingerprint: ECA1 E3F2 8E11 2432 D485 DD95 EB36 171A 6FF9 435F >> >> ------------------------------------------------------------------------------ >> Live Security Virtual Conference >> Exclusive live event will cover all the ways today's security and >> threat landscape has changed and how IT managers can respond. Discussions >> will include endpoint security, mobile security and the latest in malware >> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ >> _______________________________________________ >> Gpsbabel-code mailing list http://www.gpsbabel.org >> Gps...@li... >> https://lists.sourceforge.net/lists/listinfo/gpsbabel-code > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > Gpsbabel-code mailing list http://www.gpsbabel.org > Gps...@li... > https://lists.sourceforge.net/lists/listinfo/gpsbabel-code -- Bernd Zeimetz Debian GNU/Linux Developer http://bzed.de http://www.debian.org GPG Fingerprint: ECA1 E3F2 8E11 2432 D485 DD95 EB36 171A 6FF9 435F |
From: Robert L. <rob...@gp...> - 2012-05-17 20:15:12
|
I looked through the list and while I didn't study every one, I didn't see anything that's actually very interesting. While I do generally like tools like this, they tend to result in a lot of busy work. For example, GCC's requirement to double the parens in the perfectly semantically legal "if ((foo = bar))" doesn't really advance the state of the industry. Do we have dead writes to variables that aren't read? Yeah. So? u16 = gbfgetuint16(fin) Throwing that line away is absolutely wrong. (void) gbfgetuint16(fin) would silence the warning, but it's not actually any easier to read and I doubt that any self-respecting optimizer would generate even one opcode differently. If someone on the list wants to tackle this, great. I'll apply reasonable patches, but I'm focused on other development tasks. RJL |
From: Bernd Z. <be...@bz...> - 2012-05-18 10:46:51
|
On 05/17/2012 10:15 PM, Robert Lipe wrote: > I looked through the list and while I didn't study every one, I didn't > see anything that's actually very interesting. While I do generally > like tools like this, they tend to result in a lot of busy work. For > example, GCC's requirement to double the parens in the perfectly > semantically legal "if ((foo = bar))" doesn't really advance the state > of the industry. > > Do we have dead writes to variables that aren't read? Yeah. So? > > u16 = gbfgetuint16(fin) > > Throwing that line away is absolutely wrong. (void) gbfgetuint16(fin) > would silence the warning, but it's not actually any easier to read > and I doubt that any self-respecting optimizer would generate even one > opcode differently. Sure, that makes a lot of sense - and its not hard to make scan-build to ignore such cases. Also I found a few false-positives, That happens when you use the latest shiniest code ;) > If someone on the list wants to tackle this, great. I'll apply > reasonable patches, but I'm focused on other development tasks. I'll have a look at those which are easy or obvious to fix, but there are some issues like http://devel.recluse.de/~bzed/scan-build/gpsbabel/2012-05-18-1/report-jxOGrD.html#EndPath in mapsource.c that need knowledge about the format. -- Bernd Zeimetz Debian GNU/Linux Developer http://bzed.de http://www.debian.org GPG Fingerprint: ECA1 E3F2 8E11 2432 D485 DD95 EB36 171A 6FF9 435F |
From: Bernd Z. <be...@bz...> - 2012-05-20 10:10:12
|
Hi, here are the first few fixes for the obvious problems which were detected by scan-build. There are some more which are worth to be fixed imho, but I did not have the time to understand the (mostly uncommented) code in the few minutes I had. Maybe later :) Cheers, Bernd Bernd Zeimetz (5): gpsbabel/parse.c: ensure lathemi/lonhemi are initialized. gpsbabel/route.c: avoid dereference of null pointer gpsbabel/cst.c: Initialize wpt before using the struct. gpsbabel/util.c: Ensure a non-null argument is passed to strlen. gpsbabel/gpssim.c: '-' as filename means stdout. gpsbabel/cst.c | 2 +- gpsbabel/gpssim.c | 2 +- gpsbabel/parse.c | 3 +-- gpsbabel/route.c | 12 ++++++------ gpsbabel/util.c | 2 +- 5 files changed, 10 insertions(+), 11 deletions(-) -- 1.7.10 |
From: Bernd Z. <be...@bz...> - 2012-05-20 10:10:12
|
--- gpsbabel/route.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gpsbabel/route.c b/gpsbabel/route.c index 0085db3..67f6ba2 100644 --- a/gpsbabel/route.c +++ b/gpsbabel/route.c @@ -182,12 +182,12 @@ any_route_add_wpt(route_head *rte, waypoint *wpt, int *ct, int synth) rte->rte_waypt_ct++; /* waypoints in this route */ if (ct) { (*ct)++; - } - if (synth && !wpt->shortname) { - char tmpnam[10]; - snprintf(tmpnam, sizeof(tmpnam), "RPT%03d",*ct); - wpt->shortname = xstrdup(tmpnam); - wpt->wpt_flags.shortname_is_synthetic = 1; + if (synth && !wpt->shortname) { + char tmpnam[10]; + snprintf(tmpnam, sizeof(tmpnam), "RPT%03d",*ct); + wpt->shortname = xstrdup(tmpnam); + wpt->wpt_flags.shortname_is_synthetic = 1; + } } update_common_traits(wpt); } -- 1.7.10 |
From: Bernd Z. <be...@bz...> - 2012-05-20 10:10:12
|
--- gpsbabel/parse.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gpsbabel/parse.c b/gpsbabel/parse.c index 46807a4..1c45ee7 100644 --- a/gpsbabel/parse.c +++ b/gpsbabel/parse.c @@ -166,6 +166,7 @@ parse_coordinates(const char *str, int datum, const grid_type grid, const char *format; valid = 1; + lathemi = lonhemi = '\0'; switch (grid) { @@ -211,7 +212,6 @@ parse_coordinates(const char *str, int datum, const grid_type grid, fatal("%s: Unable to convert BNG coordinates (%s)!\n", module, str); } - lathemi = lonhemi = '\0'; break; case grid_utm: @@ -225,7 +225,6 @@ parse_coordinates(const char *str, int datum, const grid_type grid, fatal("%s: Unable to convert UTM coordinates (%s)!\n", module, str); } - lathemi = lonhemi = '\0'; break; case grid_swiss: { -- 1.7.10 |
From: Bernd Z. <be...@bz...> - 2012-05-20 10:10:14
|
--- gpsbabel/cst.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpsbabel/cst.c b/gpsbabel/cst.c index ffe8940..ec77287 100644 --- a/gpsbabel/cst.c +++ b/gpsbabel/cst.c @@ -164,7 +164,7 @@ cst_data_read(void) int cst_version; int cst_points = -1; route_head* track = NULL; - waypoint* wpt = NULL; + waypoint* wpt = waypt_new(); while ((buff = gbfgetstr(fin))) { char* cin = buff; -- 1.7.10 |
From: Bernd Z. <be...@bz...> - 2012-05-20 10:10:14
|
Don't compare an option string (which might be null) instead. --- gpsbabel/gpssim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpsbabel/gpssim.c b/gpsbabel/gpssim.c index 618463e..46e465b 100644 --- a/gpsbabel/gpssim.c +++ b/gpsbabel/gpssim.c @@ -59,7 +59,7 @@ gpssim_wr_init(const char* fname) splitfiles = splitfiles_opt ? atoi(splitfiles_opt) : 0; /* If writing to stdout, never split files */ - if (0 == strcmp("-",splitfiles_opt)) { + if (0 == strcmp("-",fname)) { splitfiles = 0; } -- 1.7.10 |
From: Bernd Z. <be...@bz...> - 2012-05-20 10:10:19
|
--- gpsbabel/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpsbabel/util.c b/gpsbabel/util.c index 65460b3..4303645 100644 --- a/gpsbabel/util.c +++ b/gpsbabel/util.c @@ -141,7 +141,7 @@ xstrdup(const char *s) #endif if (!o) { - fatal("gpsbabel: Unable to allocate %ld bytes of memory.\n", (unsigned long) strlen(s)); + fatal("gpsbabel: Unable to allocate %ld bytes of memory.\n", (unsigned long) strlen( s ? s : "")); } return o; -- 1.7.10 |