Static analysis

[Ykkrosh]

(Thought I ought to post into this forum, because it's meant to be for technical discussion but I've kind of neglected it... Any feedback would be appreciated )

Mozilla has been developing Dehydra, which is basically a way to write scripts in JavaScript that hook into the GCC compiler and analyse C++ code in whatever way you fancy. It's pretty simple to set up and run, and (most impressively) it actually works for real.

One thing it's particularly useful for is warning about certain code patterns, when the standard compiler doesn't provide the right warnings. So I spent the past couple of days learning it and writing a script to do type-checking of printf-style functions - it'll warn about printf("%s", 123) and wprintf(L"%s", L"str") etc. (GCC has some checking built in, but it doesn't handle wprintf-style (i.e. wchar_t*) functions at all (which we use a lot), and there's also some Windows compatibility issues it doesn't look at.)

I've uploaded the code here. I'm quite new to this so it's probably not particularly well written, but it seems to generally work

This system probably isn't hugely useful but I expect there's a number of cases where it's a nice way to automatically find certain classes of bugs, so it'd be good to explore how it could be used further.

In case anyone wants to bother fixing them, the messages I get from compiling the game's code now are:

../../../source/network/NetLog.cpp: In member function ‘virtual void CNetLogConsoleSink::Write(const CStr8&)’:
../../../source/network/NetLog.cpp:457: warning: Non-constant format string argument - can't check types
../../../source/ps/CConsole.cpp: In member function ‘void CConsole::InsertChar(int, wchar_t)’:
../../../source/ps/CConsole.cpp:461: warning: Non-constant format string argument - can't check types
../../../source/ps/CConsole.cpp:492: warning: Non-constant format string argument - can't check types
../../../source/ps/Interact.cpp: In member function ‘void CSelectedEntities::RenderOverlays()’:
../../../source/ps/Interact.cpp:241: error: Invalid argument type "long int" for format specifier "%d"
../../../source/ps/Interact.cpp:265: error: Invalid argument type "long int" for format specifier "%d"
../../../source/network/NetServer.cpp: In static member function ‘static void CNetServer::PlayerAttributeUpdate(const CStrW&, const CStrW&, CPlayer*, void*)’:
../../../source/network/NetServer.cpp:916: error: Invalid argument type "long unsigned int" for format specifier "%d"
../../../source/ps/GameSetup/GameSetup.cpp: In function ‘void Init(const CmdLineArgs&, int)’:
../../../source/ps/GameSetup/GameSetup.cpp:969: warning: Non-constant format string argument - can't check types
../../../source/ps/scripting/JSCollection.h: In static member function ‘static JSBool CJSCollection<T, ScriptType>::ToString(JSContext*, JSObject*, uintN, jsval*, jsval*) [with T = CPlayer*, JSClass* ScriptType = (& CJSObject<CPlayer, false>::JSI_class)]’:
../../../source/ps/scripting/JSCollection.h:90:   instantiated from ‘JSFunctionSpec CJSCollection<CPlayer*, (& CJSObject<CPlayer, false>::JSI_class)>::JSI_methods [8]’
../../../source/ps/scripting/JSCollection.h:244:   instantiated from ‘static void CJSCollection<T, ScriptType>::Init(const char*) [with T = CPlayer*, JSClass* ScriptType = (& CJSObject<CPlayer, false>::JSI_class)]’
../../../source/ps/GameSetup/GameSetup.cpp:489:   instantiated from here
../../../source/ps/scripting/JSCollection.h:266: error: Non-portable %s used in wprintf-style function
../../../source/ps/scripting/JSCollection.h:266: error: Invalid argument type "long unsigned int" for format specifier "%d"
../../../source/lib/ogl.cpp: In function ‘void ogl_WarnIfError()’:
../../../source/lib/ogl.cpp:307: warning: Non-constant format string argument - can't check types
../../../source/lib/app_hooks.cpp: In function ‘void def_log(const wchar_t*)’:
../../../source/lib/app_hooks.cpp:70: warning: Non-constant format string argument - can't check types
../../../source/simulation/Projectile.cpp: In static member function ‘static JSBool CProjectile::Construct(JSContext*, JSObject*, uintN, jsval*, jsval*)’:
../../../source/simulation/Projectile.cpp:230: warning: Non-constant format string argument - can't check types
../../../source/gui/CGUI.cpp: In member function ‘void CGUI::ReportParseError(const wchar_t*, ...)’:
../../../source/gui/CGUI.cpp:1055: warning: Non-constant format string argument - can't check types
../../../source/gui/CGUI.cpp: In member function ‘void CGUI::Xeromyces_ReadObject(XMBElement, CXeromyces*, IGUIObject*)’:
../../../source/gui/CGUI.cpp:1318: error: Invalid argument type "const short unsigned int*" for format specifier "%hs"
../../../source/gui/CGUI.cpp:1448: warning: Non-constant format string argument - can't check types
../../../source/simulation/Technology.cpp: In member function ‘bool CTechnology::LoadElEffect(XMBElement, CXeromyces&, const VfsPath&)’:
../../../source/simulation/Technology.cpp:301: error: Invalid argument type "const short unsigned int*" for format specifier "%hs"
../../../source/lib/file/file_system_util.cpp: In function ‘void fs_util::NextNumberedFilename(const PIVFS&, const VfsPath&, size_t&, VfsPath&)’:
../../../source/lib/file/file_system_util.cpp:136: warning: Non-constant format string argument - can't check types
../../../source/lib/file/file_system_util.cpp:151: warning: Non-constant format string argument - can't check types
../../../source/ps/scripting/JSCollection.h: In static member function ‘static JSBool CJSCollection<T, ScriptType>::ToString(JSContext*, JSObject*, uintN, jsval*, jsval*) [with T = HEntity, JSClass* ScriptType = (& CJSComplex<CEntity, false>::JSI_class)]’:
../../../source/ps/scripting/JSCollection.h:90:   instantiated from ‘JSFunctionSpec CJSCollection<HEntity, (& CJSComplex<CEntity, false>::JSI_class)>::JSI_methods [8]’
../../../source/ps/scripting/JSCollection.h:244:   instantiated from ‘static void CJSCollection<T, ScriptType>::Init(const char*) [with T = HEntity, JSClass* ScriptType = (& CJSComplex<CEntity, false>::JSI_class)]’
../../../source/simulation/scripting/SimulationScriptInit.cpp:46:   instantiated from here
../../../source/ps/scripting/JSCollection.h:266: error: Non-portable %s used in wprintf-style function
../../../source/ps/scripting/JSCollection.h:266: error: Invalid argument type "long unsigned int" for format specifier "%d"
../../../source/scripting/ScriptGlue.cpp: In function ‘JSBool WriteLog(JSContext*, JSObject*, uintN, jsval*, jsval*)’:
../../../source/scripting/ScriptGlue.cpp:120: warning: Non-constant format string argument - can't check types
../../../source/lib/res/graphics/ogl_tex.cpp: In function ‘LibError OglTex_to_string(const OglTex*, wchar_t*)’:
../../../source/lib/res/graphics/ogl_tex.cpp:488: error: Invalid argument type "long unsigned int" for format specifier "%x"
../../../source/lib/res/sound/snd_mgr.cpp: In function ‘LibError alc_init()’:
../../../source/lib/res/sound/snd_mgr.cpp:278: error: Non-portable %s used in wprintf-style function
../../../source/lib/sysdep/os/linux/ldbg.cpp: In function ‘LibError debug_DumpStack(wchar_t*, size_t, void*, const wchar_t*)’:
../../../source/lib/sysdep/os/linux/ldbg.cpp:115: error: Invalid argument type "void*" for format specifier "%08x"
../../../source/lib/sysdep/os/linux/ldbg.cpp:117: error: Invalid argument type "void*" for format specifier "%08x"

[janwas]

This is pure gold - thanks!

Those %hs vs %s vs %ls spots run rarely (in error output code) and are very hard to catch otherwise (slipped through my regex?!).

The non-constant format string warnings were mostly due to passing (user-controlled or output) strings as format strings, which is a total disaster for security.

I've fixed all of them except that warning in file_system_util (where the format string is a function parameter), and anything that may have been lost when the IDE crashed. Would you mind giving it another go? (no longer non-constant format strings may also add a few new ones)

[Ykkrosh]

Would you mind giving it another go?

It says:

../../../source/ps/CConsole.cpp: In member function ‘void CConsole::InsertChar(int, wchar_t)’:
../../../source/ps/CConsole.cpp:461: warning: Non-constant format string argument - can't check types
../../../source/ps/Interact.cpp: In member function ‘void CMouseoverEntities::RenderOverlays()’:
../../../source/ps/Interact.cpp:1045: error: Invalid argument type "long int" for format specifier "%d"
../../../source/lib/file/file_system_util.cpp: In function ‘void fs_util::NextNumberedFilename(const PIVFS&, const VfsPath&, size_t&, VfsPath&)’:
../../../source/lib/file/file_system_util.cpp:136: warning: Non-constant format string argument - can't check types
../../../source/lib/file/file_system_util.cpp:151: warning: Non-constant format string argument - can't check types

[janwas]

Thanks, fixed (again except for file_system_util, looks like that one will remain)

[Ykkrosh]

On a related topic, this article is interesting and slightly depressing. ("it's not uncommon for tool improvement to be viewed as "bad" or at least a problem". "You cannot often argue with people who are sufficiently confused about technical matters; they think you are the one who doesn't get it. They also tend to get emotional. Arguing reliably kills sales.")

[feneur]

I can't say I understand all of that, but from what I understand I have to agree with you Philip. No wonder there are buggy programs when these kinds of attitudes are so common

[janwas]

Wow, the "arguments" they had with developers who didn't want to believe they had bugs are indeed depressing/unbelievable (one would think they could easily be settled with a copy of the ANSI C standard).

Very interesting to hear about the real-world experience these guys had - thanks for mentioning it! On a positive note, it's good that these checkers are in more widespread use (I personally am very happy when a bug is found via static analysis, because programmers are anything but infallible).

@Erik: that reminds me of a funny Japanese saying: "If you understand everything, you must be misinformed."

Edited February 13, 2010 by janwas

[MattSherman]

"If you understand everything, you must be misinformed."

So true. Im going to use that one!