{"thread":{"id":"37178","subject":"scan.coverity: improve the modeling file of git.git","startedAt":"2014-07-20T21:44:53Z","lastAt":"2014-07-22T08:33:18Z","messageCount":2,"participants":["Stefan Beller","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"246426","messageId":"53CC3855.8050500@gmail.com","threadId":"37178","inReplyTo":null,"subject":"scan.coverity: improve the modeling file of git.git","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-07-20T21:44:53Z","receivedAt":"2014-07-20T21:44:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Sam, John and Jeff,\n\nI'm writing to you, as you're listed as the \nadministrator of the git.git project \non scan.coverity.com\n\nWe're currently seeing lots of false positives\nas the xmalloc/xrealloc function is handled not properly\nby coverity. There are lots of errors \"Allocation too small for type\"\n\nQuoting (starting linenumbers are code) from some xrealloc ocurrence:\n\n\n 95void *xrealloc(void *ptr, size_t size)\n 96{\n 97        void *ret;\n 98\n 99        memory_limit_check(size);\n100        ret = realloc(ptr, size);\n\n1. Condition \"!ret\", taking true branch   \n2. Condition \"!size\", taking true branch\n\n101        if (!ret && !size)\n   \n3. buffer_alloc: \"realloc(void *, size_t)\" which allocates 1 bytes based on \"1UL\".   \n4. var_assign: Assigning: \"ret\" = storage allocated by \"realloc(ptr, 1UL)\".\n\n102                ret = realloc(ptr, 1);\n   \n5. Condition \"!ret\", taking false branch\n\n103        if (!ret) {\n104                try_to_free_routine(size);\n105                ret = realloc(ptr, size);\n106                if (!ret && !size)\n107                        ret = realloc(ptr, 1);\n108                if (!ret)\n109                        die(\"Out of memory, realloc failed\");\n110        }\n   \n6. return_dbuffer: Returning allocated array \"ret\".\n\n111        return ret;\n112}\n\nHowever I have reviewed the function and I'd be pretty sure it would work as expected.\nAccording to https://scan.coverity.com/tune we can upload a modelling file, \nwhich will allow us to supress such false positive errors.\nI believe we'd need to put in the modelling file something like:\n\n\t// coverity[+alloc]\n\tvoid *xrealloc(void *ptr, size_t size);\n\nand that should do. We'd not need to modify the git.git sources,\nbut just add such a declaration to the modelling file.\n\nDoes anyone of you administrators want to experiment with that?\n\nCheers,\nStefan\n"},{"id":"246492","messageId":"20140722083318.GA24324@peff.net","threadId":"37178","inReplyTo":"53CC3855.8050500@gmail.com","subject":"Re: scan.coverity: improve the modeling file of git.git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-22T08:33:18Z","receivedAt":"2014-07-22T08:33:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 20, 2014 at 11:44:53PM +0200, Stefan Beller wrote:\n\n> We're currently seeing lots of false positives\n> as the xmalloc/xrealloc function is handled not properly\n> by coverity. There are lots of errors \"Allocation too small for type\"\n\nHmm. Actually, I think this report from coverity kind of makes sense.\n\nIf you pass zero bytes to xmalloc, like:\n\n  const char **foo = xmalloc(0);\n\nand your system malloc returns NULL, we convert that into a single-byte\nallocation, and it is the caller's responsibility not to actually\ndereference it. Coverity doesn't know that, and says \"wait, one byte\nisn't enough to store *foo\", which is right.\n\nMost cases of this are something like:\n\n  const char **foo = xmalloc(nr * sizeof(*foo));\n  /* do something nr times */\n\nWhat coverity _should_ be able to do is realize that we only trigger\nthis when \"nr\" is zero, and then make sure that we only dereference foo\nwhen nr is non-zero. But I guess it's not that smart (yet).\n\nAs a fallback, we should be able to say \"xrealloc is opaque; just trust\nus that it will allocate the right amount\", which I think is what you\nare getting at with the modeling file.\n\n> However I have reviewed the function and I'd be pretty sure it would work as expected.\n> According to https://scan.coverity.com/tune we can upload a modelling file, \n> which will allow us to supress such false positive errors.\n> I believe we'd need to put in the modelling file something like:\n> \n> \t// coverity[+alloc]\n> \tvoid *xrealloc(void *ptr, size_t size);\n> \n> and that should do. We'd not need to modify the git.git sources,\n> but just add such a declaration to the modelling file.\n\nI think that is how we would comment it in the source code. For the\nmodeling file, we would actually provide a \"fake\" implementation of\nxmalloc that just calls malloc. I think you shouldn't generally need to\nmodel functions which are actually in your code base (as coverity looks\nacross all files, not just what gets included when compiling any single\none), but I assume that a model would take precedence over the \"real\"\none for cases like this.\n\n> Does anyone of you administrators want to experiment with that?\n\nI don't think anybody is actively submitting builds to coverity. I\nplayed with it a while ago and have meant to look again when I got more\ntime (as there were many false positives that I figured could be removed\nwith some modeling), but haven't gotten around to it.\n\nI'm happy to upload any model file you want, or if you want to actively\nwork on this, I can bump you to \"maintainer\" status and you can fiddle\nwith the project settings as necessary.\n\n-Peff\n"}]}