Slashdot Mirror

← Back to Stories (view on slashdot.org)

PVS-Studio Analyzer Spots 40 Bugs In the FreeBSD Kernel

Posted by timothy on from the branching-dancing-regexes dept.

Andrey_Karpov writes: Svyatoslav Razmyslov from PVS-Studio Team published an article on the check of the FreeBSD kernel. PVS-Studio developers are known for analyzing various projects to show the abilities of their product, and do some advertisement, of course. Perhaps, this is one of the most acceptable and useful ways of promoting a proprietary application. They have already checked more than 200 projects and detected 9355 bugs. At least that's the number of bugs in the error base of their company.

So now it was FreeBSD kernel's turn. The source code was taken from GitHub 'master' branch. Svyatoslav states that PVS-Studio detected more than 1000 suspicious code fragments that are most likely bugs or inaccurate code. He described 40 of them in the article. The list of warnings was given to the FreeBSD developer team and they have already started editing the code.

A couple of words for programmers who are still not familiar with PVS-Studio. PVS-Studio is a tool for bug detection in the source code of programs, written in C, C++ and C#. It performs static code analysis and generates a report that helps a programmer find and fix the errors in the code. You can see a more detailed description of the tool on the company website and download a trial version.

26 of 169 comments (clear)

  1. ahhhh advertising, my good friend! by muphin · · Score: 2, Insightful

    you're looking at spending about $5k for the product, unless you are a large development team, cost benefit ratio is low

    --
    It's not a typo if you understood the meaning!
    1. Re:ahhhh advertising, my good friend! by gstoddart · · Score: 5, Insightful

      You know, if you want "free" advertising by doing free code analysis against a piece of free software, publish your results openly, and give them the output to the project to actually use to improve that project ... you're bloody welcome to some free advertising.

      Depending on the software you write, and what you use it for ... $5k for a development tool isn't that crazy stupid.

      One with proven results against a known piece of software and which contributes to eliminate bugs in a provable way and gives those results freely to open source?

      Oh, hell yeah, bring on the free advertising for more companies like this. And hopefully people are thinking "holy crap, if they found over a 1000 questionable pieces in the FreeBSD kernel, imagine what they can do with my stuff".

      I say kudos to these guys, and any "free" advertising (beyond their time invested and the value of giving back to the FreeBSD project) is deservedly theirs.

      --
      Lost at C:>. Found at C.
    2. Re:ahhhh advertising, my good friend! by vux984 · · Score: 5, Insightful

      So far every thing I've seen in their analysis is a bug in their software

      How far did you read the article? Starting with the second example, they were finding things that were not logically correct.

      For example

      if ((m->m_flags & M_PKTHDR) == 0 ||
                  m->m_pkthdr.len != m->m_pkthdr.len { ...

      That or clause is clearly defective.

      and the very first one, rather than being a FreeBSD bug is a style bug that just looks bad, but is working as intended, yet they intentionally mislead by indicating that its a flaw. Its not, its badly formatted, but its working as intended and that if statement is only meant to control the first line.

      I disagree. It doesn't just look bad, it's indentation is communicating semantics that aren't accurate. It should be corrected. Something that should be corrected... is a flaw.

      You say its "working as intended" (and I presume it is); but the message the developer communicated with that formatting is that he intended for it to work differently from how it does in fact work.

      I agree its "just a formatting error"... but its a particularly nasty one; and code like that SHOULD be investigated and corrected.

    3. Re:ahhhh advertising, my good friend! by Dutch+Gun · · Score: 5, Insightful

      Formatting is important - it indicates to human programmers what the *intent* of code is supposed to be, at least in whitespace-neutral languages like C. This doesn't sound like a bug in the analysis software. I would definitely want a product to flag (albeit with low priority) any instances of that sort of misleading indentation in my code, because either it works correctly but looks wrong, or it works incorrectly but looks fine. The former is less serious than the latter obviously, but both should be fixed, IMO.

      The rest of the article is worth a read, even if you disagree with the first style-related issues. There are a lot of other issues that can only be definitively labeled as bugs by the BSD developers who know the codes, but if they aren't bugs, they sure look like them. There are cases where both branches lead to duplicate, identical code being executed. There are null pointer checks that come after the pointer dereference. There are flags set that do nothing. There are variables corrupted because operator precedence was misunderstood. Even if some of these happen to work correctly, it's likely only because of chance or it's in rarely exercised code. And worse, fragile code means it's more likely to break in the future when minor changes are made.

      All in all, it's a fairly impressive list of finds, at least from an outside perspective. I'd be curious to see how many of these are deemed as bugs by the BSD and get fixed.

      --
      Irony: Agile development has too much intertia to be abandoned now.
    4. Re:ahhhh advertising, my good friend! by Anonymous Coward · · Score: 3, Informative

      Depending on the software you write, and what you use it for ... $5k for a development tool isn't that crazy stupid.

      Agreed, it's not crazy stupid at all. In fact, I think it's pretty reasonable when you think about it.
      For comparison, I remember when JBuilder (the Java IDE) cost at least that much money _per seat_. And that was a long time ago.

      And on the advertising topic, this kind of advertising doesn't bother me at all. It's proving the product, factual, and relevant. What more can you ask for?

    5. Re:ahhhh advertising, my good friend! by arth1 · · Score: 3, Insightful

      And on the advertising topic, this kind of advertising doesn't bother me at all. It's proving the product, factual, and relevant. What more can you ask for?

      That it's not deceptive, but presents itself as advertising.
      Writing "They have already checked ..." when it's really "We have ..." is deliberately misleading, and I prefer honesty.

      Sure, new powers that be, bring on slashvertisements, as it can be useful, but mark them as such, and avoid astroturfing, with submissions pretending to be an enthused user.

      Honesty in advertising - I know, what a concept. But here, I think it would work better. The curmudgeon user base here likely prides itself on never getting to the once in "fool me once, shame on me", but discards anything that smells of deceptiveness or social engineering. Even in marketing.

    6. Re:ahhhh advertising, my good friend! by gstoddart · · Score: 5, Insightful

      LOL ... aww, that's sweet.

      So, yeah -- hate corporate douchebags and morons, can't fault anybody who gets product promotion by actually proving the product works and giving the results for free to a high profile bit of free software to make it better. Who knew?

      I don't hate the entire world, just huge swaths of it made up of assholes and idiots. The good bits still make me happy, but we seldom see those.

      Maybe it's a coherent outrage based on moral principles and reasoned thought? That, or the meds finally worked today, who knows.

      Slashdot posts plenty of things which require outrage -- this particular "Slashvertisement" is pretty much the exact opposite. It's showing you have something of value by proving it works, and contributing to something and making it better. If that leads to sales and revenue, best of luck.

      So, world -- "philanth-ver-tize" more, and grumpy, bitter old men might say "wow, that's awesome". Go ahead, I fucking dare you to give us a few things to be positive about. ;-)

      Cheers

      --
      Lost at C:>. Found at C.
    7. Re:ahhhh advertising, my good friend! by Vertigo+Acid · · Score: 2

      You think the FreeBSD foundation has 0 funds and the github page is their primary web presence? Lol....

      --
      Beta is bad enough to make me go edit settings like this sig that haven't been touched since I joined
    8. Re:ahhhh advertising, my good friend! by Vertigo+Acid · · Score: 3, Funny

      I disagree. It doesn't just look bad, it's indentation is communicating semantics that aren't accurate. It should be corrected. Something that should be corrected... is a flaw.

      You say its "working as intended" (and I presume it is); but the message the developer communicated with that formatting is that he intended for it to work differently from how it does in fact work.

      I agree its "just a formatting error"... but its a particularly nasty one; and code like that SHOULD be investigated and corrected.

      Thank god FreeBSD isn't written in python

      --
      Beta is bad enough to make me go edit settings like this sig that haven't been touched since I joined
    9. Re:ahhhh advertising, my good friend! by Immerman · · Score: 2

      Fairly certain they were being sarcastic, and allowing code-indent to differ from code-intent would be a style guide violation.

      But if not, then I have to agree - I'd be *very* suspicious of that software's quality. Not because I ever intend to look at the code, but because all of the people that *do* regularly look at the code will have a strong tendency to read what the visual formatting says is going on, rather than what the punctuation actually indicates. An invitation to what is gently called "unespected behavior" lest hearing it's true name drive you mad.

      Set the standard to whatever you want, but adhere to it with 100% consistency. Otherwise you create situations where, to invoke the mandatory car analogy, the lines down the road curve to the left, while the road itself veers sharply right. Sooner or later there will be hell to pay.

      --
      --- Most topics have many sides worth arguing, allow me to take one opposite you.
    10. Re:ahhhh advertising, my good friend! by TapeCutter · · Score: 3, Interesting

      The very best that can be said about the code snippet is that it is a redundant if statement. The last time someone independently ran a static analyser on something I was working on was the Y2K thing. I sent off one MB of zipped source as requested, a month or so later I got back fifteen MB of zipped reports. It cost the company a small fortune to confirm what we had told them in the first instance - dates were all handled via a handful of functions in a single source file. The entire team of ~50 developers saw the analysis as a complete waste of time and money, the report was longer and more difficult to review than the actual code. The reason it was done is the company executives (and the law) saw it as insurance via due diligence.

      Having said that, static analysis can be a very useful tool for improving code quality, if (and only if) you understand the application you are looking at.

      --
      And did you exchange a walk on part in the war for a lead role in a cage? - Pink Floyd.
    11. Re:ahhhh advertising, my good friend! by Dahan · · Score: 4, Interesting

      If you want something better, there's Coverity. Free if you qualify. If not, it's even more expensive than PVS-Studio, but does a heck of a lot better job.

      FreeBSD has been analyzed by Coverity for years... did it not catch the problems that PVS-Studio found?

    12. Re: ahhhh advertising, my good friend! by arglebargle_xiv · · Score: 2

      cppcheck is kinda the budget version of PVS, it catches a lot of the things that PVS does, and in fact there's some cross-pollination between the two. Definitely one of the must-have tools in your dev process if you can't afford PVS.

    13. Re:ahhhh advertising, my good friend! by arglebargle_xiv · · Score: 3, Insightful

      Well that's easy enough, you just don't put the bugs in there in the first place. For example my code is mostly bug-free, I insert a small number of carefully-placed ones for Dave in Q&A to find and then we split the bug bounty between us.

    14. Re:ahhhh advertising, my good friend! by DrXym · · Score: 4, Insightful
      I've spent a lot of time tracking down bugs which turn out to be stupid coding errors. e.g. one recent example was a piece of code doing a strcpy on a string into a tooltip struct without limiting the length. The copy overran the struct and caused heap corruption and a crash on exit. So the bug happened in one place, the crash happened somewhere else.

      I ran the VS2015 built-in code analysis tools, which didn't find the issue but did highlight some dubious looking code in other places which I fixed while I was at it. So there is merit in code analysis, even if it didn't help me in this instance. I eventually found the issue by plastering crt heap debug calls all over until I isolated the place where the corruption happened.

      And some code analysis tools have proven to be a total waste of time. I recall using Purify / Quantify in one workplace hoping to isolate a runtime issue where it put so much instrumentation over the code that it took 10x as long to build and ended up crashing for its own reasons. It wasted more of my time than it would have taken to fix the issue without its "help". In my experience the more expensive a development tool is, the more bugs and the less benefit it will bestow from its use - and if it's from IBM then it will be massively expensive and bestow zero benefit.

    15. Re:ahhhh advertising, my good friend! by TheRaven64 · · Score: 4, Informative

      Coverity, along with most static analysers, has problems with false positives. I spent an afternoon wading through the coverity reports in my own code in FreeBSD libc. I found one possible bug (I think that the code was unreachable, but I may have missed something in one of the callers), all of the rest were false positives. Some were trivial to discount - they were simple artefacts of the fact that Coverity works one compilation unit at a time and a cursory glance at the other compilation unit showed that it was not a problem. The others required a bit more digging. In particular, Coverity seemed to be really confused by some reference counting functions.

      I've only had time to glance over the PVS results, but they seemed to be more useful.

      --
      I am TheRaven on Soylent News
  2. Poor Practices by PVS Studio and HexRays by ilikenwf · · Score: 3, Interesting

    It seems like every time they do this for promotion they just claim everything as a "bug" without really individually investigating and reporting all of them, taking only some obviously wrong ones and then lumping the whole report onto the project's bug tracker, if we're lucky.

    PVS Studio is a great application but since they only do team licensing "1-9 developers" I can't see the benefit in buying it, just like IDA Pro. I'm an open source only dev in the C/C++/C# world, all my profitable work is in other languages...

    I'd gladly pay a REASONABLE price for all these tools if they'd not only provide proper Linux versions (PVS studio only ever had an internal Linux version...in projects with Linux and Windows specific code it is difficult if not impossible to analyze the Linux parts) but so far since it seems like the real benefit to open source teams who can't afford this software (that is windows only anyway, mostly) is extremely low despite it's utility otherwise.

  3. This ought to fix most of that ... :-) by fahrbot-bot · · Score: 3, Funny

    PVS-Studio detected more than 1000 suspicious code fragments that are most likely bugs or inaccurate code.

    /*NOTREACHED*/

    --
    It must have been something you assimilated. . . .
  4. Two character comments are good ... by perpenso · · Score: 2

    There are variables corrupted because operator precedence was misunderstood.

    One of my favorite (not) type of bugs. Because a "two character comment", a pair of parenthesis, would just be awful. Two character to document your intent, which hopefully matches your implementation, but if not may just save you.

    1. Re:Two character comments are good ... by TheRaven64 · · Score: 4, Interesting

      This is one of my pet peeves too. It's also something that I really like about Smalltalk: there is no operator precedence, operators are evaluated left to right and if you want something other than left-to-right order, then you must add parentheses. This means that you never spend time in Smalltalk code wondering if the developer got the precedence wrong.

      --
      I am TheRaven on Soylent News
  5. Bethesda Softworks by Merovign · · Score: 5, Funny

    Somebody get this to Bethesda, stat!

  6. BitZtream was wrong. They just fixed it. by Anonymous Coward · · Score: 4, Interesting

    BitZtream was wrong. A fix has been committed which adds the missing parenthesis.

  7. Re:he's way overconfident by ledow · · Score: 2

    Then at minimum you'd expect removal of the check (not a comment), or a history of patches which indicate that it was actually a deliberate omission after testing.

  8. Re:Lol they lead with goto-fail by Viol8 · · Score: 2

    Tabs are bloody useful for indentation since people can set the tab width to whatever they want when viewing code. Good luck doing that with spaces.

  9. Re:Dear PVD Team: by Andrey_Karpov · · Score: 2

    See "An always up-to-date list of articles describing errors that we find in open source projects with PVS-Studio analyzer".

    We have checked this projects from the list you've provided:

  10. Re:Oh, Karpov, you inveterate spammer... by Andrey_Karpov · · Score: 3, Interesting

    You may just say - hey this is me, psychonaut, I've banned viva64 on Wikipedia. Praise me for that. Because of me you won't see links to really helpful material on viva64.

    For example, it's really not necessary for those who are interested in Precompiled header to know that there is a super useful article StdAfx.h. Burn it all! :)