Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#11195 closed defect (fixed)

scan result of static code analysis tool

Reported by: ettl.martin Owned by:
Priority: low Milestone: 2.9.0
Component: GUI-all Version: 2.9.0
Keywords: cppcheck Cc: lanurmi@…
Blocked By: Blocking:
Patch: no

Description

Hi friends,

i have scanned the current version of wxWidgets-2.9.0 with the static code analysis tool cppcheck. It found a few (not much) issues, that might be easy to fix. This could help to make the wxWidgets library even more stable.

Here is the result of the scan

../expat/xmlwf/xmlfile.c,183,Error,Resource leak: fd
../expat/xmlwf/readfilemap.c,59,Error,Resource leak: fd
../tiff/libtiff/tif_ojpeg.c,925,Error,Invalid number of character ((). Can't process file.
../tiff/tools/ras2tiff.c,158,Error,Resource leak: in
../tiff/tools/raw2tiff.c,215,Error,Resource leak: fd
../tiff/contrib/iptcutil/iptcutil.c,297,Error,Memory leak: str
../tiff/contrib/iptcutil/iptcutil.c,570,Error,Resource leak: ifile
../tiff/contrib/iptcutil/iptcutil.c,570,Error,Resource leak: ofile
../motif/menu.cpp,379,Error,Invalid number of character ({). Can't process file.
../os2/spinctrl.cpp,129,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 170
../os2/spinctrl.cpp,130,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 170
../os2/spinbutt.cpp,66,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 115
../os2/spinbutt.cpp,67,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 115
../os2/listctrl.cpp,86,Error,Invalid number of character ((). Can't process file.
../osx/carbon/treectrl.cpp,19,Error,Invalid number of character ((). Can't process file.
../msw/ole/activex.cpp,673,Error,Invalid number of character ({). Can't process file.
../png/pngtest.c,411,Error,Invalid number of character ((). Can't process file.
../propgrid/propgrid.cpp,4699,Error,Dangerous usage of erase
../propgrid/property.cpp,2120,Error,Same iterator is used with both children and m_children
../../samples/mediaplayer/mediaplayer.cpp,759,Style,C-style pointer casting
../../samples/mediaplayer/mediaplayer.cpp,1430,Error,Possible null pointer dereference: currentpage - otherwise it is redundant to check if currentpage is null at line 1432
../../samples/memcheck/memcheck.cpp,121,possible error,Memory leak: brush
../../samples/memcheck/memcheck.cpp,121,Error,Memory leak: ordinaryNonObject

Best regards

Martin

Change History (16)

comment:1 follow-up: Changed 11 years ago by vadz

  • Resolution set to invalid
  • Status changed from new to closed

Sorry, I simply don't see anything useful in the list above so I don't really understand what's the problem here -- are we expected to fix bugs in the tool you use (indicate by "Can't process file" messages)?

Anyhow, if there are any real problems indicated by the above messages, please point them out, I could have missed something. But dumping the output here without any explanations is not particularly helpful.

comment:2 Changed 11 years ago by SN

  • Resolution changed from invalid to fixed

(In [61904]) Be more paranoid about parent window possibly being NULL (partly fixes #11195).

comment:3 in reply to: ↑ 1 Changed 11 years ago by neis

Replying to vadz:

Sorry, I simply don't see anything useful in the list above so I don't really understand what's the problem here -- are we expected to fix bugs in the tool you use (indicate by "Can't process file" messages)?

Actually, the warnings about OS/2's spinbutton/-control possibly dereferencing a null pointer (or not needing the null pointer check further down) where correct, IMHO, although it's probably unlikely that somebody creates such a control with a null parent?
Anyway, just to be on the safe side, I've added a check.

comment:4 Changed 11 years ago by vadz

Controls must always have parents and wxWindowBase::CreateBase() checks for this so if you use it you should be already ok. And if you don't, you should [use it].

comment:5 Changed 11 years ago by lanurmi

  • Cc lanurmi@… added

Vadim, you are being quite negative against a bug report that after all lists a number of actual potential errors, with exact line numbers and human-readable explanations.

At least one of those "Can't process file" messages is caused by an extra ')' in png/pngtest.c.

comment:6 follow-up: Changed 11 years ago by vadz

I'm being negative because clearly zero effort was put into this ticket creation, it's really just a dump of the tool output AFAICS. And no, this is not useful.

comment:7 in reply to: ↑ 6 Changed 11 years ago by ettl.martin

Replying to vadz:

I'm being negative because clearly zero effort was put into this ticket creation, it's really just a dump of the tool output AFAICS. And no, this is not useful.

Ignoring the output of static code analysis tools is also not useful! Maybe not every warning is corret, but it is worth to take a look into the code and not rececting it!.... Cheers

comment:8 Changed 11 years ago by vadz

Once again, I'd be glad and thankful if you made this analysis and submitted only relevant or at least possibly relevant warnings. As you didn't even try to do it I have no intention of doing it myself considering I have no idea what tool did you use nor whether I should trust it. If I had time to spend on this I'd rather fix warnings given by coverity which did find some real problems in wx code in the past.

Anyhow, I can only repeat that this ticket (as well as the previous one for the code which is not used by wx at all) was just not helpful at all. In fact it was actively harmful because it resulted in wasting time of everyone involved so I'm going to at least stop doing this.

comment:9 follow-up: Changed 11 years ago by aggro80

  • Resolution fixed deleted
  • Status changed from closed to reopened

He did mention that he used Cppcheck ( http://sourceforge.net/apps/mediawiki/cppcheck/ ). I'm one of the developers of Cppcheck and I looked into that list a little.

../expat/xmlwf/xmlfile.c,183,Error,Resource leak: fd

-- false positive (only with --all)

../expat/xmlwf/readfilemap.c,59,Error,Resource leak: fd

-- close(fd); is not called in that function in all error situations when exiting the function.

../tiff/libtiff/tif_ojpeg.c,925,Error,Invalid number of character ((). Can't process file.

The code which is protected by "#ifdef never" looks something that can't be compiled. E.g. the code below has uneven amount of "()".

        if (   (sp->cinfo.c.comp_info[0].component_id = s) == 1)
            && sp->cinfo.c.jpeg_color_space == JCS_YCbCr
           )
../tiff/tools/ras2tiff.c,158,Error,Resource leak: in
../tiff/tools/raw2tiff.c,215,Error,Resource leak: fd
../tiff/contrib/iptcutil/iptcutil.c,297,Error,Memory leak: str
../tiff/contrib/iptcutil/iptcutil.c,570,Error,Resource leak: ifile
../tiff/contrib/iptcutil/iptcutil.c,570,Error,Resource leak: ofile

First one is a leak. Others I didn't check manually as I doubt that you will fix issues in tools/contrib.

../motif/menu.cpp,379,Error,Invalid number of character ({). Can't process file.

That code won't compile if this #if is not true, because the "}" is on the wrong side of the #endif.

#if (XmVersion >= 1002)
        if ( menu->IsTearOff() )
        {
            XtVaSetValues(GetWidget(menu),
                          XmNtearOffModel, XmTEAR_OFF_ENABLED,
                          NULL);
            Widget tearOff = XmGetTearOffControl(GetWidget(menu));
            wxDoChangeForegroundColour((Widget) tearOff, m_foregroundColour);
            wxDoChangeBackgroundColour((Widget) tearOff, m_backgroundColour, true);
#endif
        }
----
../os2/spinctrl.cpp,129,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 170
../os2/spinctrl.cpp,130,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 170
../os2/spinbutt.cpp,66,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 115
../os2/spinbutt.cpp,67,Error,Possible null pointer dereference: pParent - otherwise it is redundant to check if pParent is null at line 115

False positives, fixed in latest Cppcheck trunk.

../os2/listctrl.cpp,86,Error,Invalid number of character ((). Can't process file.

Line number is a bit off, but this member function does not have ")" after the "int nImage":

// Sets the item image
bool wxListCtrl::SetItemColumnImage (
  long                              lItem
, long                              lColumn
, int                               nImage
{
    wxListItem                      vInfo;

    vInfo.m_mask   = wxLIST_MASK_IMAGE;
    vInfo.m_image  = nImage;
    vInfo.m_itemId = lItem;
    vInfo.m_col    = lColumn;
    return SetItem(vInfo);
} // end of wxListCtrl::SetItemColumnImage
../propgrid/property.cpp,2120,Error,Same iterator is used with both children and m_children

False positive, fixed in latest Cppcheck trunk.

../propgrid/propgrid.cpp,4699,Error,Dangerous usage of erase

AFAIK, erase on std::map invalidates the iterator and here the iterator is used after erase, so this looks like a potential problem. But I don't know what wxPGHashMapI2I is. Do you think this is a false positive?

void wxPropertyGrid::ClearActionTriggers( int action )
{
    wxPGHashMapI2I::iterator it;

    for ( it = m_actionTriggers.begin(); it != m_actionTriggers.end(); ++it )
    {
        if ( it->second == action )
        {
            m_actionTriggers.erase(it);
        }
    }
}
../osx/carbon/treectrl.cpp,19,Error,Invalid number of character ((). Can't process file.

Error is actually in this line:

        wxFAIL_MSG(wxT("unknown action in wxTreeCtrl::ExpandItem");
../msw/ole/activex.cpp,673,Error,Invalid number of character ({). Can't process file.
../png/pngtest.c,411,Error,Invalid number of character ((). Can't process file.

I didn't check these yet.

../../samples/mediaplayer/mediaplayer.cpp,1430,Error,Possible null pointer dereference: currentpage - otherwise it is redundant to check if currentpage is null at line 1432

This is real issue (but in sample code). currentpage is used and after that null check is done for it.

    wxMediaCtrl* currentMediaCtrl = currentpage->m_mediactrl;

    if(currentpage)
../../samples/mediaplayer/mediaplayer.cpp,759,Style,C-style pointer casting
../../samples/memcheck/memcheck.cpp,121,possible error,Memory leak: brush
../../samples/memcheck/memcheck.cpp,121,Error,Memory leak: ordinaryNonObject

I didn't check these as they are sample code.

I'm reopening this ticket as I assume I solved all the reasons for closing this ticket.

comment:10 Changed 11 years ago by aggro80

../png/pngtest.c,411,Error,Invalid number of character ((). Can't process file.

The error is in the macro definition. See the double "))"

#  define WRITEFILE(file, data, length, check)) \
../msw/ole/activex.cpp,673,Error,Invalid number of character ({). Can't process file.

This is actually a bug in Cppcheck preprocessor. I created a ticket for Cppcheck about it:
http://sourceforge.net/apps/trac/cppcheck/ticket/667

And about "warnings given by coverity which did find some real problems in wx code in the past". Cppcheck has found problems before also. I just didn't remember to mention Cppcheck at that time:
http://trac.wxwidgets.org/ticket/10234

FYI: Other bugs found by Cppcheck and been reported to you that I'm aware of are listed here (I haven't been updating it lately, not sure if others have):
https://sourceforge.net/apps/mediawiki/cppcheck/index.php?title=Found_bugs#wxWidgets

comment:11 Changed 11 years ago by vadz

  • Priority changed from normal to low
  • Summary changed from scan resulut of static code analysis tool to scan result of static code analysis tool
  • Type changed from optimization to defect

I don't have anything against cppcheck but it's useless to run it against the code which is never compiled (as part of wx, anyhow). This applies to all 3rd party library code (see #11194) and probably the files containing syntax errors (like mismatching braces or missing parentheses).

IOW the only valid issue I see is the one in mediaplayer sample and I'll fix it (by simply removing the check as it appears to be unneeded).

Irrespectively of this, the analysis you did was helpful (and very different from the original submission), thanks for looking into this.

comment:12 Changed 11 years ago by aggro80

Could you move all 3rd party code under a folder called "3rdparty" or similar. It is not obvious to me that folder called "os2" contains 3rd party code. I would have never investigated those issues if the code had been located like that.

comment:13 Changed 11 years ago by vadz

"os2" isn't 3rd party, but the only error in it is in src/os2/listctrl.cpp which is clearly not used (don't ask me why is it there, I don't know much about OS/2 port) as it contains an obvious syntax error.

FWIW I agree that it would have been better to have png, tiff, jpeg and so on under a separate 3rdparty directory but it's really not worth the trouble to move them now.

comment:14 Changed 11 years ago by lanurmi

I doubt "motif" and "osx" are 3rd party either.

comment:15 Changed 11 years ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [61917]) Remove unneeded pointer check.

This was flagged as an error by static code analyse tools.

Closes #11195.

comment:16 in reply to: ↑ 9 Changed 11 years ago by jmsalli

Replying to aggro80:

../propgrid/propgrid.cpp,4699,Error,Dangerous usage of erase AFAIK, erase on std::map invalidates the iterator and here the iterator is used after erase, so this looks like a potential problem. But I don't know what wxPGHashMapI2I is. Do you think this is a false positive?

This was indeed an incorrect use of map::erase(). Should be fixed now in SVN trunk.

Thanks!

Note: See TracTickets for help on using tickets.