Taking the inspiration of Valeriy Kravchuk great series of blog posts “Fun with Bugs” (and not http://funwithbugs.com/ which is about both caring for and eating bugs), and since I recently went and run Coverity against Drizzle, I thought I’d have a small series of posts on bugs that it has found (and I’ve fixed).
An idea that has been pervasive in the Drizzle project (and one that I rather subscribe to) is that there is two types of correct: correct and obviously correct. Being obviously correct is much, much better than merely being correct.
The first category of problems that Coverity found was kind of interesting, there was a warning that data_file_name and index_file_name in class ha_myisam weren’t initialized in the ha_myisam constructor nor in any function that it calls. It turns out that this was basically because the code wasn’t exactly optimal, and these variables were used kind of oddly. In fact, in writing this blog post I went back and found that there’s a bunch of extra dead code and these should just be removed, along with the code that “used” them.
The historical use for data_file_name and index_file_name were that (in MySQL) you could specify different paths for MyISAM data and index files, so that the FRM ended up in the server datadir, the data file ended up some other place and the index file was off behind the sofa. Since MyISAM is used only for temporary tables in Drizzle, this is entirely not needed.
Another place where a similar bug was found by Coverity was in the SQLExecutor class of the json_server plugin. The _err variable wasn’t initialized in the constructor. After some careful auditing, I think this was actually a false positive as it was set to something before being used, but it was pretty simple to prevent future bugs by initializing it.
Two instances of the same warning, one just found a bunch of code to delete (rather useful) and the other is rather minor but may help someone in the future.
Coming up next: total embarrassment bugs.
Mark Atwood liked this on Facebook.
Matt Lord liked this on Facebook.
Valerii Kravchuk liked this on Facebook.
False positives are the bane of Coverity. Running Cyrus through it I found one CVE and O(10^3) false positives. For example it really doesn’t like strcpy and sprintf even when it has enough information to predict that they’re safe.
We’re lucky in that we have very few sprintf users left in Drizzle and the ones we do have it is possible to go and verify that they are correct. That being said, I think I prefer us to go in and convert them to snprintf (or std::string) anyway, with the hope that the snprintf code can be obviously correct rather than “if we look at everything carefully it’s correct”
If you’re getting reports about strcpy and sprintf, that’s because you’ve turned on the checker that’s deliberately meant to be a grep-like thing. If you don’t want that, just turn it off (it’s the poorly named SECURE_CODING checker). Unlike almost all other Coverity checkers, this checker is merely grep on function names, as opposed to finding likely real defects. It was built because one customer demanded that we build it – not because we thought it was a particularly good idea.
In the next version, SECURE_CODING won’t be turned on by the “–all” flag by default. Using that option is how most people get into trouble. So now “all” means “all useful checkers” not really “all checkers”.
At least for Drizzle, I’ve simply done the default way as outlined on the instructions for scan.coverity.com…. and it has given us enough to look at without trying other options :)
There’s a Microsoft header banned.h which I hope to some day actually include in our code.