Bug: IDAPython Alt+7

It is well known that one of the principles of “good programming” is the separation of code into functions, classes, namespaces, files, etc. There are many explanations for this separation. One reason is the idea that a bunch of codelines (or logic) should not appear more then once in the code so it will be possible to modify the code only once to affect all parts in the code that use the same logic. This principle is well described and known simply as DRY (Don’t Repeat Yourself) and DIE (Duplication Is Evil).

I worked on an IDAPython project where another colleague added GUI to the project’s interface. The person who wrote the GUI left the company without the time to finish the GUI feature, so we got left with a half-done GUI that could only run one time per IDA session. Runing the GUI script twice resulted in a crash for the script and for IDA.

At first, while debugging the script in order to find the cause for this devious bug, we had to reopen IDA every time. Later – we discovered that reopening the IDB without reopening IDA was enough. This was a good hint for us that perhaps the problem was not within IDA but somewhere else.

After a while we discovered that the cause for the crash was within our very own code. The GUI code relied on values from sys.argv to determine the relative path of the images for the GUI. The first time the script ran the value of sys.argv, it accurately contained the path of the script, but on second run it was a list containing an empty string ([”]).

Now we must ask, why was sys.argv modified and by whom? At the end of the first run its value was the same as in the beginning. Before jumping into IDAPython’s source code we ran a little test.

import sys
print sys.argv

We saved this two-liner script twice under different names and ran them one after another and did not receive empty string in result. Running one of the scripts a second time using [Alt + 7] gave the familiar error of empy string in sys.argv.

Let us look at the source:

This is the code that runs by pressing [Alt + 9] (\python\init.py)

def runscript(script):
    """
    Run the specified script after adding its directory path to
    system path.
    This function is used by the low-level plugin code.
    """
    addscriptpath(script)
    watchdog.reset()
    argv = sys.argv
    sys.argv = [ script ]
    execfile(script, globals())
    sys.argv = argv

* Notice the proper handling of sys.argv to emulate a real script running.

Now look at what happens at [Alt + 7] (somewhere at the source of IDAPython in a .cpp file):

/* History of previously executed scripts */
/* Default hotkey: Alt-7 */
void IDAPython_ScriptBox(void)
{
    PyObject *module;
    PyObject *dict;
    PyObject *scriptbox;
    PyObject *pystr;
    /* Get globals() */
    /* These two should never fail */
    module = PyImport_AddModule("__main__");
    dict = PyModule_GetDict(module);
    scriptbox = PyDict_GetItemString(dict, "scriptbox");
    if (!scriptbox)
    {
        warning("INTERNAL ERROR: ScriptBox_instance missing! Broken init.py?");
        return;
    }
    pystr = PyObject_CallMethod(scriptbox, "run", "");
    if (pystr)
    {
        /* If the return value is string use it as path */
        if (PyObject_TypeCheck(pystr, &PyString_Type))
        {
            begin_execution();
            ExecFile(PyString_AsString(pystr));
            end_execution();
        }
    Py_DECREF(pystr);
}

Not only is the [alt+7] logic implemented in a different code file then the [alt-9] one (although the two have a strong connection), but also it is missing some of the logic of [alt+9], the logic that stores the values of argv for every script.

Therefore, this was not the right way to implement it.

*Sidenote, this bug was found on IDAPython 1.0 and 1.2.