Opened 9 years ago

Closed 9 years ago

Last modified 8 months ago

#5448 closed defect (fixed)

SCUMM: Code analysis warnings

Reported by: Templier Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
Keywords: build Cc:
Game:

Description

Here are the warnings I get when compiling the engine using Code Analysis in VS2010. I tried to remove all the obvious false positives. Some warnings look suspicious, others are pointing to correct code that still might benefit from better error checking.

d:\sources\scummvm\scummvm\engines\scumm\script_v5.cpp(1851): warning C6308: 'realloc' might return null pointer: assigning null pointer to 'ptr', which is passed as an argument to 'realloc', will cause the original memory block to be leaked
d:\sources\scummvm\scummvm\engines\scumm\script_v5.cpp(1853): warning C6011: Dereferencing NULL pointer 'ptr'
d:\sources\scummvm\scummvm\engines\scumm\scumm.cpp(139): warning C6031: Return value ignored: 'sscanf'
d:\sources\scummvm\scummvm\engines\scumm\sound.cpp(1749): warning C6326: Potential comparison of a constant with another constant
d:\sources\scummvm\scummvm\engines\scumm\object.cpp(1001): warning C6011: Dereferencing NULL pointer 'imhd'
d:\sources\scummvm\scummvm\engines\scumm\palette.cpp(501): warning C6290: Bitwise operation on logical result: ! has higher precedence than &. Use && or (!(x & y)) instead
d:\sources\scummvm\scummvm\engines\scumm\palette.cpp(547): warning C6290: Bitwise operation on logical result: ! has higher precedence than &. Use && or (!(x & y)) instead
d:\sources\scummvm\scummvm\engines\scumm\smush\smush_player.cpp(101): warning C6011: Dereferencing NULL pointer 'id_start-1'
d:\sources\scummvm\scummvm\engines\scumm\debugger.cpp(90): warning C6240: (<expression> && <non-zero constant>) always evaluates to the result of <expression>. Did you intend to use the bitwise-and operator?
d:\sources\scummvm\scummvm\engines\scumm\insane\insane.cpp(949): warning C6200: Index '43' is out of valid index range '0' to '10' for non-stack buffer 'int const * const `private: bool __thiscall Scumm::Insane::actor1StateFlags(int)'::`2'::spans'
d:\sources\scummvm\scummvm\engines\scumm\insane\insane.cpp(1104): warning C6200: Index '35' is out of valid index range '0' to '8' for non-stack buffer 'int const * const `private: bool __thiscall Scumm::Insane::actor0StateFlags1(int)'::`2'::spans'
d:\sources\scummvm\scummvm\engines\scumm\insane\insane.cpp(1124): warning C6200: Index '403' is out of valid index range '0' to '100' for non-stack buffer 'int const * const `private: bool __thiscall Scumm::Insane::actor0StateFlags2(int)'::`2'::spans'
d:\sources\scummvm\scummvm\engines\scumm\insane\insane_enemy.cpp(1331): warning C6287: Redundant code: the left and right sub-expressions are identical
d:\sources\scummvm\scummvm\engines\scumm\smush\codec47.cpp(305): warning C6200: Index '511' is out of valid index range '0' to '509' for non-stack buffer 'codec47_table'
d:\sources\scummvm\scummvm\engines\scumm\smush\codec47.cpp(305): warning C6200: Index '510' is out of valid index range '0' to '509' for non-stack buffer 'codec47_table'
d:\sources\scummvm\scummvm\engines\scumm\he\floodfill_he.cpp(278): warning C6011: Dereferencing NULL pointer 'wizd+py*w+px'
d:\sources\scummvm\scummvm\engines\scumm\he\resource_he.cpp(285): warning C6011: Dereferencing NULL pointer 'lang_wr'
d:\sources\scummvm\scummvm\engines\scumm\he\resource_he.cpp(699): warning C6308: 'realloc' might return null pointer: assigning null pointer to 'fi->memory', which is passed as an argument to 'realloc', will cause the original memory block to be leaked

Note:
If you want to try it yourself, you will need:
- the create_project patch set, along with the WIP code analysis patch (http://bitbucket.org/Littleboy/scummvm-jt/src)
- the following patch to silence some false positives ( http://bitbucket.org/Littleboy/scummvm-jt/src/tip/COMMON%20-%20Add%20annotations%20for%20analysis%20build%20configuration.patch )
- Visual Studio 2010 Ultimate or Team System (create_project support for analysis with VS2005/2008 isn't complete yet)

Ticket imported from: #3087898. Ticket imported from: bugs/5448.

Change History (9)

comment:1 Changed 9 years ago by fingolfin

Thanks for the report. I am going to take a stab at fixing them.

The warning in smush/codec47.cpp is particularly interesting. It seems our codec47_table is missing two entries (or _table is slightly too big?). I always kind of wondered where that 510 came from... Pawel, since you REd that code originally, could you take a look at this?

comment:2 Changed 9 years ago by fingolfin

Owner: set to aquadran

comment:3 Changed 9 years ago by fingolfin

OK, I addressed most of these. Several indicate genuine bugs (some looking quite nasty). A few seem like incorrect warnings, though (e.g. the one in he/floodfill_he.cpp complains about wizd+py*w+px possibly being NULL, but wizd is checked with an assert before that for being non-NULL).
Regardless, I really like this code analysis feature :).

There is another one that should be dealt with, but that I don't know enough: Namely
insane/insane_enemy.cpp:1331 warning C6287: Redundant code: the left and right sub-expressions are identical

The code in question is this:
if (!_actor[1].lost && !_actor[1].lost) {

Maybe one of the two "1"s should be a 0 ? Eugene, I think you REd that code, maybe you could take a peek at that?

Julien, could you please re-run the analysis? Thanks!

comment:4 Changed 9 years ago by fingolfin

Owner: changed from aquadran to sev-

comment:5 Changed 9 years ago by Templier

Here is what's left after your last commit (r53567):

d:\sources\scummvm\scummvm\engines\scumm\script_v5.cpp(1851): warning C6308: 'realloc' might return null pointer: assigning null pointer to 'ptr', which is passed as an argument to 'realloc', will cause the original memory block to be leaked
d:\sources\scummvm\scummvm\engines\scumm\script_v5.cpp(1854): warning C6011: Dereferencing NULL pointer 'ptr': Lines: 1619, 1620, 1622, 1627, 1628, 1823, 1827, 1828, 1830, 1831, 1834, 1835, 1840, 1841, 1842, 1843, 1844, 1845, 1854
d:\sources\scummvm\scummvm\engines\scumm\sound.cpp(1749): warning C6326: Potential comparison of a constant with another constant
d:\sources\scummvm\scummvm\engines\scumm\smush\smush_player.cpp(101): warning C6011: Dereferencing NULL pointer 'id_start-1': Lines: 89, 90, 91, 92, 94, 95, 99, 100, 101
d:\sources\scummvm\scummvm\engines\scumm\insane\insane_enemy.cpp(1331): warning C6287: Redundant code: the left and right sub-expressions are identical
d:\sources\scummvm\scummvm\engines\scumm\smush\codec47.cpp(305): warning C6200: Index '511' is out of valid index range '0' to '509' for non-stack buffer 'codec47_table'
d:\sources\scummvm\scummvm\engines\scumm\smush\codec47.cpp(305): warning C6200: Index '510' is out of valid index range '0' to '509' for non-stack buffer 'codec47_table'
d:\sources\scummvm\scummvm\engines\scumm\he\resource_he.cpp(701): warning C6308: 'realloc' might return null pointer: assigning null pointer to 'fi->memory', which is passed as an argument to 'realloc', will cause the original memory block to be leaked

Here is the page about C6308: http://msdn.microsoft.com/en-us/library/kkedhy7c%28VS.90%29.aspx
While asserting makes sure it will crash at the point of error, it doesn't fix the warning (you will have to use a temporary for that). Since the same pattern appears in several other places, maybe adding a macro will be easier.

comment:6 Changed 9 years ago by fingolfin

Fixed the (harmless) issue in codec47.cpp (thanks to pawel for the help), and also the uses of realloc -- I hope.
Eugene is looking into the INSANE issue.

comment:7 Changed 9 years ago by fingolfin

Owner: changed from sev- to fingolfin
Resolution: fixed
Status: newclosed

comment:8 Changed 9 years ago by fingolfin

Eugene just fixed the bug in FT, which really was a bug. Great catch!

comment:9 Changed 8 months ago by digitall

Component: Engine: SCUMM
Note: See TracTickets for help on using tickets.