How to manage hundreds of bugs found by static analysis of Inkscape source?
I've used Coverity's static source code analysis tools to identify hundreds of bugs in Inkscape source. I'd like some guidance on the best way to contribute bug report details and proposed fixes to Inkscape. I'm a relatively new Coverity employee getting up to speed with our static analysis tools for identifying defects. Inkscape was recommended as an open source project with C/C++ source that I could use to satisfy my educational needs while contributing to an open source project.
I want to contribute code fixes along with bug details. I suggest a first pass to report as many bugs as I can to the Inkscape Bug Tracker, followed by a second pass to add suggested fixes. Then Inkscape developers with commit privileges can review my code fix suggestions and apply the fixes as they see fit. Once the first pass is done and bug details are visible to the community, anyone else is welcome to contribute fixes also. What process should we follow to make the best use of everyone's efforts by avoiding several people working on the same bug?
What I'm requesting at this point is a discussion about my suggestions, and some decisions about how best to proceed. As an example, here's one defect found by Coverity analysis, with my proposed code fix.
CONSTANT_EXPRESSION_RESULT in /inkbugs/inkscape/src/main.cpp
In sp_do_export_png(SPDocument *...): An operation with non-constant operands that computes a result with constant value (CWE-569).
1345 if (sp_export_area) {
1346 /* Try to parse area (given in SVG pixels) */
1347 gdouble x0,y0,x1,y1;
!sscanf(sp_export_area, &"%lg:%lg:%lg:%lg", &x0, &y0, &x1, &y1) == 4 is always false regardless of the values of its operands (logical operand of if).
1348 if (!sscanf(sp_export_area, "%lg:%lg:%lg:%lg", &x0, &y0, &x1, &y1) == 4) {
1349 g_warning("Cannot parse export area '%s'; use 'x0:y0:x1:y1'. Nothing exported.", sp_export_area);
1350 return;
1351 }
fix noted 2010-07-25:
1348 if (sscanf(sp_export_area, "%lg:%lg:%lg:%lg", &x0, &y0, &x1, &y1) != 4) {
fix reason:
sscanf returns the number of strings successfully scanned. The intent here is to ensure that all 4 input strings were scanned successfully.
Vaughn Spurlin
Coverity Technical Support
Vaughn Spurlin wrote:
I’ve used Coverity’s static source code analysis tools to identify hundreds of bugs in Inkscape source. I’d like some guidance on the best way to contribute bug report details and proposed fixes to Inkscape. I’m a relatively new Coverity employee getting up to speed with our static analysis tools for identifying defects. Inkscape was recommended as an open source project with C/C++ source that I could use to satisfy my educational needs while contributing to an open source project.
Nice to hear! It will be interesting to see what kinds of bugs you unearth and what kind of impact that will have on the user experience. (Anyone got ideas on how we could analyse the impact this will have?)
I want to contribute code fixes along with bug details. I suggest a first pass to report as many bugs as I can to the Inkscape Bug Tracker, followed by a second pass to add suggested fixes. Then Inkscape developers with commit privileges can review my code fix suggestions and apply the fixes as they see fit. Once the first pass is done and bug details are visible to the community, anyone else is welcome to contribute fixes also. What process should we follow to make the best use of everyone’s efforts by avoiding several people working on the same bug?
Is it possible to categorize bugs? It could even help to simply break them up w.r.t. where in the code they occur. For example, I might be more comfortable reviewing and applying bugfixes for code that in src/display (and some other areas), while other people might be more comfortable with fixing bugs in lib2geom, lpe's, certain areas of the gui, etc.
Also, in the slightly longer run it could be appropriate to give you Bazaar access to allow for a more direct path.
What I’m requesting at this point is a discussion about my suggestions, and some decisions about how best to proceed. As an example, here’s one defect found by Coverity analysis, with my proposed code fix.
It might be good to communicate with the Inkscape "bug team" directly and have a look at the guidelines for reporting bugs in general: http://wiki.inkscape.org/wiki/index.php/Bug_management
With regard to the specific bug you posted in your e-mail, I'm currently rebuilding Inkscape, but as soon as that's finished I'll have a look and apply your fix, so I guess that's one down :)
Hi Vaughn,
I think that it is a great offer to help identify and fix Inkscape bugs!! One that we should definitely facilitate. The question is how best to do this? The biggest problem, that I see, is that as Inkscape is developed by volunteers, the handling of hundreds of bugs may overwhelm the community! Here are some ideas that could help in this regard:
1. Can the bugs be grouped by type and just one bug report be opened for each type?
2. Inkscape has a policy that after a person has had two patches accepted the person is given write access to the code repository. Perhaps after submitting a few fixes, you can apply fixes directly to the code.
I do want to say that I really like your bug report. It clearly identifies the problem, the fix, and, most importantly for me, the rational for the fix.
Tav
On Mon, 2010-08-02 at 23:10 -0700, Vaughn Spurlin wrote:
I’ve used Coverity’s static source code analysis tools to identify hundreds of bugs in Inkscape source. I’d like some guidance on the best way to contribute bug report details and proposed fixes to Inkscape. I’m a relatively new Coverity employee getting up to speed with our static analysis tools for identifying defects. Inkscape was recommended as an open source project with C/C++ source that I could use to satisfy my educational needs while contributing to an open source project.
I want to contribute code fixes along with bug details. I suggest a first pass to report as many bugs as I can to the Inkscape Bug Tracker, followed by a second pass to add suggested fixes. Then Inkscape developers with commit privileges can review my code fix suggestions and apply the fixes as they see fit. Once the first pass is done and bug details are visible to the community, anyone else is welcome to contribute fixes also. What process should we follow to make the best use of everyone’s efforts by avoiding several people working on the same bug?
What I’m requesting at this point is a discussion about my suggestions, and some decisions about how best to proceed. As an example, here’s one defect found by Coverity analysis, with my proposed code fix.
CONSTANT_EXPRESSION_RESULT in /inkbugs/inkscape/src/main.cpp
In sp_do_export_png(SPDocument *…): An operation with non-constant operands that computes a result with constant value (CWE-569).
1345 if (sp_export_area) {
1346 /* Try to parse area (given in SVG pixels) */
1347 gdouble x0,y0,x1,y1;
!sscanf(sp_export_area, &"%lg:%lg:%lg:%lg", &x0, &y0, &x1, &y1) == 4 is always false regardless of the values of its operands (logical operand of if).
1348 if (!sscanf(sp_export_area, "%lg:%lg:%lg:%lg", &x0, &y0, &x1, &y1) == 4) {
1349 g_warning("Cannot parse export area '%s'; use 'x0:y0:x1:y1'. Nothing exported.", sp_export_area);
1350 return;
1351 }
fix noted 2010-07-25:
1348 if (sscanf(sp_export_area, "%lg:%lg:%lg:%lg", &x0, &y0, &x1, &y1) != 4) {
fix reason:
sscanf returns the number of strings successfully scanned. The intent here is to ensure that all 4 input strings were scanned successfully.
Vaughn Spurlin
Coverity Technical Support
The Palm PDK Hot Apps Program offers developers who use the Plug-In Development Kit to bring their C/C++ apps to Palm for a share of $1 Million in cash or HP Products. Visit us here for more details: http://p.sf.net/sfu/dev2dev-palm _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Hi Vaughn,
I like the code checking too! When you decide adding all the bugs (grouped or not) to our bug tracker, I think you should add a specific tag "static code analysis" or something, such that they can easily be found.
Tav wrote:
Perhaps after submitting a few fixes, you can apply fixes directly to the code.
+1 (if Coverity allows you to do that?)
Tav wrote:
I do want to say that I really like your bug report. It clearly identifies the problem, the fix, and, most importantly for me, the rational for the fix.
Indeed, the bug report is quite clear (although it does not mention the (imho) real reason for the bug, C++ operator precedence, which may not be so clear to an inexperienced programmer). But I don't think you should write out such a report for each bug. (I'm guessing that the Coverity software provided the first part of the report?)
Great news!
Ciao, Johan
-----Original Message----- From: Tavmjong Bah [mailto:tavmjong@...8...] Sent: Tuesday, August 03, 2010 13:28
Hi Vaughn,
I think that it is a great offer to help identify and fix Inkscape bugs!! One that we should definitely facilitate. The question is how best to do this? The biggest problem, that I see, is that as Inkscape is developed by volunteers, the handling of hundreds of bugs may overwhelm the community! Here are some ideas that could help in this regard:
- Can the bugs be grouped by type and just one bug report be
opened for each type?
- Inkscape has a policy that after a person has had two
patches accepted the person is given write access to the code repository. Perhaps after submitting a few fixes, you can apply fixes directly to the code.
I do want to say that I really like your bug report. It clearly identifies the problem, the fix, and, most importantly for me, the rational for the fix.
TavOn Mon, 2010-08-02 at 23:10 -0700, Vaughn Spurlin wrote:
I've used Coverity's static source code analysis tools to identify hundreds of bugs in Inkscape source. I'd like some guidance on the best way to contribute bug report details and proposed fixes to Inkscape. I'm a relatively new Coverity employee getting up
to speed
with our static analysis tools for identifying defects.
Inkscape was
recommended as an open source project with C/C++ source
that I could
use to satisfy my educational needs while contributing to an open source project.
I want to contribute code fixes along with bug details. I suggest a first pass to report as many bugs as I can to the Inkscape Bug Tracker, followed by a second pass to add suggested fixes. Then Inkscape developers with commit privileges can review my code fix suggestions and apply the fixes as they see fit. Once the
first pass
is done and bug details are visible to the community,
anyone else is
welcome to contribute fixes also. What process should we follow to make the best use of everyone's efforts by avoiding several people working on the same bug?
What I'm requesting at this point is a discussion about my suggestions, and some decisions about how best to proceed. As an example, here's one defect found by Coverity analysis, with my proposed code fix.
CONSTANT_EXPRESSION_RESULT in /inkbugs/inkscape/src/main.cpp
In sp_do_export_png(SPDocument *...): An operation with
non-constant
operands that computes a result with constant value (CWE-569).
1345 if (sp_export_area) {
1346 /* Try to parse area (given in SVG pixels) */
1347 gdouble x0,y0,x1,y1;
!sscanf(sp_export_area, &"%lg:%lg:%lg:%lg", &x0, &y0, &x1,
&y1) == 4
is always false regardless of the values of its operands (logical operand of if).
1348 if (!sscanf(sp_export_area, "%lg:%lg:%lg:%lg",
&x0, &y0,
&x1, &y1) == 4) {
1349 g_warning("Cannot parse export area '%s'; use 'x0:y0:x1:y1'. Nothing exported.", sp_export_area);
1350 return;
1351 }
fix noted 2010-07-25:
1348 if (sscanf(sp_export_area, "%lg:%lg:%lg:%lg", &x0, &y0, &x1, &y1) != 4) {
fix reason:
sscanf returns the number of strings successfully scanned. The intent here is to ensure that all 4 input strings were scanned successfully.
Vaughn Spurlin
Coverity Technical Support
On 3/8/10 13:54, J.B.C.Engelen@...1578... wrote:
I like the code checking too! When you decide adding all the bugs (grouped or not) to our bug tracker, I think you should add a specific tag "static code analysis" or something, such that they can easily be found.
Spaces can't be used in tags with Launchpad's bug tracker -> this would add 3 separate tags which would not really help with the search and by themselves don't make much sense. Why not use 'sca' as tag or even 'coverity'?
Grouping many issues by type into one bug report may make it difficult to track what parts have been committed already - OTOH filing hundreds of new bug reports will likely overwhelm the capacity of active Inkscape developers to review & commit, and leave many open reports in the bug tracker.
What about using a branch for the changes, thus being able to test compatibility across platforms and with different compiler versions without risks, and then merge with Inkscape trunk?
~suv
The branch idea only works if people on other platforms and compilers actually test it. As we've shown several times now, that just doesn't happen (dbus springs to mind...)
Sent from my iPhone
On 3 Aug 2010, at 13:56, ~suv <suv-sf@...58...> wrote:
On 3/8/10 13:54, J.B.C.Engelen@...1578... wrote:
I like the code checking too! When you decide adding all the bugs (grouped or not) to our bug tracker, I think you should add a specific tag "static code analysis" or something, such that they can easily be found.
Spaces can't be used in tags with Launchpad's bug tracker -> this would add 3 separate tags which would not really help with the search and by themselves don't make much sense. Why not use 'sca' as tag or even 'coverity'?
Grouping many issues by type into one bug report may make it difficult to track what parts have been committed already - OTOH filing hundreds of new bug reports will likely overwhelm the capacity of active Inkscape developers to review & commit, and leave many open reports in the bug tracker.
What about using a branch for the changes, thus being able to test compatibility across platforms and with different compiler versions without risks, and then merge with Inkscape trunk?
~suv
The Palm PDK Hot Apps Program offers developers who use the Plug-In Development Kit to bring their C/C++ apps to Palm for a share of $1 Million in cash or HP Products. Visit us here for more details: http://p.sf.net/sfu/dev2dev-palm _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On Mon, 2010-08-02 at 23:10 -0700, Vaughn Spurlin wrote:
What I’m requesting at this point is a discussion about my suggestions, and some decisions about how best to proceed. As an example, here’s one defect found by Coverity analysis, with my proposed code fix.
Hi Vaughn,
I think you got people a little excited about potential contributions. :) There is one thing that no one has brought up so far. We currently have three ongoing Google Summer of Code projects that will be modifying significant portions of the codebase. So what you have run analysis on is probably significantly different than what the codebase will look like in a month from now. I'm not trying to discourage you, just giving a heads up. :)
Cheers, Josh
Hi all,
I'm delighted with the quick and enthusiastic response. This obviously is a great community to work with. Instead of responding to all the suggestions right now, I'm going to ponder this and come up with a specific plan later today. Meanwhile, more ideas are definitely helpful. The better I understand how this project and its people operate, the more effective my plan will be.
More soon, and thanks for the welcome! Vaughn
-----Original Message----- From: Joshua A. Andler [mailto:scislac@...400...] Sent: Tuesday, August 03, 2010 9:30 AM To: Vaughn Spurlin Cc: inkscape-devel@lists.sourceforge.net Subject: Re: [Inkscape-devel] How to manage hundreds of bugs found by static analysis of Inkscape source?
On Mon, 2010-08-02 at 23:10 -0700, Vaughn Spurlin wrote:
What I’m requesting at this point is a discussion about my suggestions, and some decisions about how best to proceed. As an example, here’s one defect found by Coverity analysis, with my proposed code fix.
Hi Vaughn,
I think you got people a little excited about potential contributions. :) There is one thing that no one has brought up so far. We currently have three ongoing Google Summer of Code projects that will be modifying significant portions of the codebase. So what you have run analysis on is probably significantly different than what the codebase will look like in a month from now. I'm not trying to discourage you, just giving a heads up. :)
Cheers, Josh
Here's my plan for getting started, to be adjusted based on experience.
I've requested membership in the Inkscape Bug Team. That seems like a more focused group for discussion about bugs found by Coverity analysis.
I agree with ~suv that "coverity" is a good tag for these bugs. I'm OK with any other tag the bug team prefers.
I'll just start filing bugs as soon I find out what tag to use. After filing a bunch, I'll come back and add comments with a suggested fix. I don't expect to fix them all, so anyone else is welcome to contribute their fix first. I'll alternate between adding bugs and adding fixes.
Bugs found by static analysis (SA) are significantly different from bugs found by functional testing or by using the product. One could say SA takes an inside out or bottom up view, where usage testing is more top down or outside inward. The techniques complement very nicely. Static analysis can find critical defects quickly that are extremely difficult to find by testing or code review.
I don't have the broad knowledge of Inkscape that's needed to verify and test my suggested fixes. Others with that knowledge should review my suggested source changes, apply or reject the changes, and test them. For example, if I find code that's never executed, the programmer probably intended a very different result. Instead of simply deleting the dead code, someone with broader knowledge might make a different change that would make the code serve its correct purpose.
Also, the Inkscape code base continues to evolve rapidly. The Google Summer of Code projects will make some bug reports moot, while adding new code that needs review. The Coverity tools are designed to keep track of the same bug when code around it changes, and to remove bugs that belong to code that was removed. So I'll just go ahead with what I've got, and others can decide whether or not to proceed with the bug fixes I suggest. I'll update and reanalyze the source occasionally.
That's my plan for now. As soon as I get a tag approved by the bug team, I'll start adding bugs.
Vaughn
-----Original Message-----
I'm delighted with the quick and enthusiastic response. This obviously is a great community to work with. Instead of responding to all the suggestions right now, I'm going to ponder this and come up with a specific plan later today. Meanwhile, more ideas are definitely helpful. The better I understand how this project and its people operate, the more effective my plan will be.
More soon, and thanks for the welcome! Vaughn
On Wed, 2010-08-04 at 09:31 -0700, Vaughn Spurlin wrote:
I agree with ~suv that "coverity" is a good tag for these bugs. I'm OK with any other tag the bug team prefers.
I think that tag is good as well.
I'll just start filing bugs as soon I find out what tag to use. After filing a bunch, I'll come back and add comments with a suggested fix. I don't expect to fix them all, so anyone else is welcome to contribute their fix first. I'll alternate between adding bugs and adding fixes.
While filing bugs, if you are going to change the "importance", please be sure to read the following wiki page to know how we rank things. Since what you are doing is from the inside out, the rankings may differ. However, if there is anything that has the potential to cause crashes or memory leaks, I would just go with High for those. http://wiki.inkscape.org/wiki/index.php/Bug_management
I don't have the broad knowledge of Inkscape that's needed to verify and test my suggested fixes. Others with that knowledge should review my suggested source changes, apply or reject the changes, and test them. For example, if I find code that's never executed, the programmer probably intended a very different result. Instead of simply deleting the dead code, someone with broader knowledge might make a different change that would make the code serve its correct purpose.
When adding the fixes to the bug reports, please try to attach diffs if it's convenient.
Also, the Inkscape code base continues to evolve rapidly. The Google Summer of Code projects will make some bug reports moot, while adding new code that needs review. The Coverity tools are designed to keep track of the same bug when code around it changes, and to remove bugs that belong to code that was removed. So I'll just go ahead with what I've got, and others can decide whether or not to proceed with the bug fixes I suggest. I'll update and reanalyze the source occasionally.
The merges will be coming in the near future (I imagine in the next month it should all be merged), so it will be interesting to see. :)
That's my plan for now. As soon as I get a tag approved by the bug team, I'll start adding bugs.
You are approved. :)
Cheers, Josh
On Aug 4, 2010, at 9:31 AM, Vaughn Spurlin wrote:
Here's my plan for getting started, to be adjusted based on experience.
I've requested membership in the Inkscape Bug Team. That seems like a more focused group for discussion about bugs found by Coverity analysis.
I agree with ~suv that "coverity" is a good tag for these bugs. I'm OK with any other tag the bug team prefers.
I'll just start filing bugs as soon I find out what tag to use. After filing a bunch, I'll come back and add comments with a suggested fix. I don't expect to fix them all, so anyone else is welcome to contribute their fix first. I'll alternate between adding bugs and adding fixes.
Hi Vaughn. Thanks for the assistance here.
I think, though, that we might want to hold off on a slew of bug reports right off the bat. There are several ways to address things, and many of them should be corrected by more architectural changes.
However... I think the main point would be to see if Coverity still can facilitate access to the runs and live instance itself. Do you know anything about this? Anyway, it's much much nicer to actually interact with Coverity's data and tracking directly so that we can do things such as spot patterns, compare runs, flag various issues, etc.
Among other things, getting ongoing analysis running will help pretty quickly. *Especially* with how dynamic our codebase will be, and how spotty the paid work to correct issues is (aka all volunteer).
Oh, and I agree with you about the details that SA can bring, and how the issues such as dealing with dead code are addressed. We've had to restore things a few times that people had attributed to code being dead that wasn't, and were missing the big picture to catch the real solution. It's definitely an area we want to keep focused and help with.
And, yes, I've used Coverity and other tools in the past. I definitely like the product, and the SA it does can really help more than most expect. We're actually at a good point for that now. Over the last few years one of the things we've been doing has been to increase our warning levels and fix compile time errors as they can be looked at. Now that the bulk of those are settled we can step it up a notch and benefit from some good SA help.
2010/8/3 Vaughn Spurlin <vspurlin@...2386...>:
I’ve used Coverity’s static source code analysis tools to identify hundreds of bugs in Inkscape source. I’d like some guidance on the best way to contribute bug report details and proposed fixes to Inkscape. I’m a relatively new Coverity employee getting up to speed with our static analysis tools for identifying defects. Inkscape was recommended as an open source project with C/C++ source that I could use to satisfy my educational needs while contributing to an open source project.
Hello
Here's some more specific stability information. The code in libnr/ and display/ will change a lot after we merge this year's GSoC work. Files prefixed with sp- in the main directory might also change, but probably to a lesser extent. Code in 2geom/ is an old in-tree copy of lib2geom and will be replaced soon; if you want to work on it as well, the current version is at lp:lib2geom. Other code is rather stable for now.
Regards, Krzysztof
participants (9)
-
unknown＠example.com -
Jasper van de Gronde -
John Cliff -
Jon Cruz -
Joshua A. Andler -
Krzysztof Kosiński -
Tavmjong Bah -
Vaughn Spurlin -
~suv