Fwd: Suspicious code in amarok revision 862370

[Mark Kretschmann]

Hey guys,

I was happy to receive this mail today, containing a code defect
analysis of Amarok SVN.

If you fix any of these issues, please reply to this mail stating what
issue you have fixed, as to prevent duplicated work.


HF :)


---------- Forwarded message ----------
From: Christoph Bartoschek <bartoschek at or.uni-bonn.de>
Date: Mon, Sep 22, 2008 at 1:14 AM
Subject: Suspicious code in amarok revision 862370
To: kretschmann at kde.org


Hi,

You asked for a static analysis of amarok's code. Here are some of the results
of an IBM internal tool on some of amaork's code. Some code is not checked
because it is not C++ according to the EDG compiler and some items I've
filtered out because they do not look like bugs.

- amarok/src/context/plasma/animator.cpp:291

Division by 0 if frames < 3

- amarok/src/context/plasma/containment.cpp:991

Line 986 indicates that focusedApplet can be NULL. But focusedApplet is equal
to
applet. Therefore applet can be NULL here.

- amarok/src/context/widgets/ContainmentArrow.cpp:98

If no case is selected then widht and height are uninitialized. Maybe a
default
case with an assertion would be good.

- amarok/src/dynamic/BiasSolver.cpp:492
- amarok/src/dynamic/BiasSolver.cpp:513

If there are no m_biases then activeBiases is 0. The (double) cast is needless
here.

- amarok/src/dynamic/BiasSolver.cpp:605

This memory is not freed. The pointer is lost by leaving the scope.

- amarok/src/collection/support/MemoryMatcher.cpp:51

m_next is always NULL here because you filter other values out by line 50.
I guess it's a memory leak. BTW, there is no need to check for NULL before
doing
delete.

- amarok/src/playlistmanager/PlaylistManager.cpp:266

The message printed should be more descriptive because in the default case
line
269 crashes.

- amarok/src/App.cpp:904

KRun::runCommand now returns a bool. No need for an arithmetic check.

- amarok/src/MainWindow.cpp:367

Qt::controlModifier is 0x04000000 and !e->modifiers() is either 0 or 1.
Therefore the result is always 0.

- src/browsers/servicebrowser/lastfm/meta/LastFmMeta.cpp:466

Meta::Capability::SourceInfo has the value 8. Therefore the whole expression
is
always true.

- amarok/src/browsers/servicebrowser/lastfm/core/Radio.cpp:392

RadioError has no value 20. Playlist_InvalidSession has value 20. This is at
least bad style.

- src/context/applets/lastfmevents/LastFmEvents.cpp:109

Line 98 indicates that m_theme can be NULL. A crash follows here.

- amarok/src/context/applets/serviceinfo/ServiceInfo.cpp:116
 amarok/src/context/applets/wikipedia/WikipediaApplet.cpp:133

Line 109 indicates that m_header can be NULL. A crash follows here.

- amarok/src/context/containments/ColumnContainment.cpp:511

Line 489 indicates that m_switchLeftIcon can be NULL. A crash follows here.



-- 
Mark Kretschmann
Amarok Developer
www.kde.org - amarok.kde.org