#5448 closed defect (fixed)
SCUMM: Code analysis warnings
| Reported by: | Templier | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | 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 by , 14 years ago
comment:2 by , 14 years ago
| Owner: | set to |
|---|
comment:3 by , 14 years ago
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 by , 14 years ago
| Owner: | changed from to |
|---|
comment:5 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
| Owner: | changed from to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:9 by , 5 years ago
| Component: | → Engine: SCUMM |
|---|
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?