PVS-Studio Analyzer Spots 40 Bugs In the FreeBSD Kernel

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.

Comments

ahhhh advertising, my good friend!

[muphin]

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

Re:ahhhh advertising, my good friend!

[gstoddart]

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.

Re:ahhhh advertising, my good friend!

[vux984]

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.

Re:ahhhh advertising, my good friend!

[Dutch Gun]

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.

Re:ahhhh advertising, my good friend!

[Anonymous Coward]

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?

Re:ahhhh advertising, my good friend!

[arth1]

$5 is bullshit if you're an open source project with 0 funds and a github page.

Oh, I think $5 is manageable, but $5k is a different story.
And if you're a github developer, you might not be using Microsoft Visual Studio, the only thing this tool runs under.

Can't one just analyze the C/C++ code under Windows then? Not necessarily, no. The code may depend on #ifdef paths that are or are not taken under Windows, defining different macros. It will almost certainly have different bits.h and similar includes. That can (and in many cases will) lead to both false positives and missed bugs, which means the product just can't be trusted. Which you can tell from the "bugs" the authors claim to have found in BSD and Linux code, which aren't, in fact, bugs at all.
A human can certainly spend time on eyeballing the finds for false positives after scanning, but you can do nothing about bugs that were missed due to the architecture differences.

If you just want some basic and free for C code, there's splint. Better than nothing at an unbeatable price.

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.

If you are Windows developer, and fall between those two, sure, it may be worth a look. But it's not a silver bullet, and paying someone $5k to spend a couple of weeks poring over the code for bugs might be just as fruitful.

Re:ahhhh advertising, my good friend!

[arth1]

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.

Re:ahhhh advertising, my good friend!

[gstoddart]

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

Re:ahhhh advertising, my good friend!

[arth1]

wait WHAT? FreeBSD developers have a coding style that permits, let alone encourages, the indentations not to track the actual structure of their code?

They have a ":ts=4" requirement, unless I am mistaken. Which means that if your tabs indent 8 characters, you are in error when reading it.
Sure, it's better to use spaces instead of tabs, but if your standard says that tabs equivalent a certain amount of fill space, what's default in Visual Studio should have absolutely no bearing on the correctness if you differ from that.

Re:ahhhh advertising, my good friend!

[Lunix Nutcase]

They do share the results with the project they analyze. They specifically mention that in every one of these articles they write.

Re:ahhhh advertising, my good friend!

[Vertigo Acid]

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

Re:ahhhh advertising, my good friend!

[Vertigo Acid]

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

Re:ahhhh advertising, my good friend!

[Immerman]

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.

Re:ahhhh advertising, my good friend!

[TapeCutter]

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.

Re:ahhhh advertising, my good friend!

[Dahan]

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?

Re:ahhhh advertising, my good friend!

[TapeCutter]

With windows or any other O/S the analyser assumes the code compiles on that O/S, I don't think cross-compiles are supported as a general rule. The only thing the analyser does not have direct access to via the source code are the #defines passed in an the command line. I have no idea why anyone would tie their analyser to visual other than to take advantage of the "app store" infrastructure.

Re:ahhhh advertising, my good friend!

[arglebargle_xiv]

It's not even obviously $5K, in a field (source code analysis) that's notorious for high prices and opaque pricing practices, PVS is one of the worst offenders, Try finding out what it'd cost to get a long-term license for use by an open-source project of the kind they analyse and publish articles on. I mean an actual hard figure, not a wooly estimate taken from some vague terms on a web page.

Re: ahhhh advertising, my good friend!

[arglebargle_xiv]

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.

Re:ahhhh advertising, my good friend!

[arglebargle_xiv]

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.

Re:ahhhh advertising, my good friend!

[drolli]

$5K =~ 25-50developer hours.

All SW i work with is either free or costs more than 5K per seat. (I think that the SW licenses used by me should account to about $10k-$20k/year)

If i believe that a tool licensed for as little as $5k helps me in doing my job (and the job of colleagues) more efficiently, the money usually is not the problem.

Re:ahhhh advertising, my good friend!

[Archtech]

It all depends how many people rely on the product, and how important it is to them. If it's a piece of avionics that keeps thousands of airliners flying safely, I'd suggest that $5k is not a lot. In such a case, $5 billion would be money well spent if it made the product significantly safer and more reliable.

Please remember, too, that to many corporate executives $5k is - quite literally - lunch money. Or at least the cost of a good dinner with some "decent" wine.

Re:ahhhh advertising, my good friend!

[Anonymous Coward]

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.

Exactly. This triggered all sorts of warning bells inside my head because this is one of the ways you hide malicious code in a large software project.

Re:ahhhh advertising, my good friend!

[Andrey_Karpov]

It doesn't have any prospects to make a cheap tool. I suggest looking at CppCat story, a tool we were selling for $250.

Re:ahhhh advertising, my good friend!

[arglebargle_xiv]

If you just want some basic and free for C code, there's splint. Better than nothing at an unbeatable price.

Unfortunately it's also guaranteed to find at least 20x as many problems as there actually are, so it's only free if the developer time spent weeding out FPs is also free. If you want a free alternative to PVS, go for cppcheck.

Re:ahhhh advertising, my good friend!

[arglebargle_xiv]

Even the first example is clearly a bug, it registers the Padlock entropy source if the appropriate VIA CPUID flag indicates its presence, but deregisters it unconditionally. It's impressive that it found that, I don't know of any other tool that would check that.

(Just about to submit a feature request to cppcheck...).

Re:ahhhh advertising, my good friend!

[arglebargle_xiv]

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

A bit like goto fail, which PVS should have caught had it been used on the code.

Its possible that this check in PVS was actually inspired by goto fail, hard to tell without the devs letting us know.

Re:ahhhh advertising, my good friend!

[AmiMoJo]

It really depends on the type of project. In this case, many of the issues look like merge errors. Someone submitted a patch, someone else merged it at some point and some curly brackets were lost or a comparison was broken or operator precedence was ignored.

I see that sort of thing a lot in projects where there has been a lot of merging, and of course open source projects are often the worst for it. An open source tool that could do this would actually be of great benefit I think. It's a shame they don't do a free version, or maybe partner with someone like GitHub to offer it as a (free) service.

Re:ahhhh advertising, my good friend!

[DrXym]

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.

Re:ahhhh advertising, my good friend!

[arth1]

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

There are over 10,000 outstanding defects in the Coverity scan, so who knows. I'm not going to go through all of them to find out. :-)

Also, Coverity scanning levels might be set to discard "cosmetic" defects or defects that are optimized away, like doing the same tests twice (which appears to be a Big Thing for PVS-Studio) or initializing a variable twice.

Re:ahhhh advertising, my good friend!

[TheRaven64]

Coverity is free for open source projects, you can sign up from their web page and they'll scan your repo (and email out reports, or let you view them from the web interface). It's only expensive if you want to run it on your own systems or for code that isn't public.

Re:ahhhh advertising, my good friend!

[TheRaven64]

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.

Re:ahhhh advertising, my good friend!

[TheRaven64]

You can set a prettyprinter to support just about any formatting standard you prefer these deays. (How many spaces in a tab, braces on end of line or separate lines and so on)

I've still not found one that supports my coding style, which uses one tab for each indent level and spaces for alignment (so you can adjust the tab width in the editor and still have correctly formatted code, whatever value you pick). Actually, that's not quite true: one of my students wrote one that did just that (and use the TeX line-breaking algorithm with different penalties for different syntactic features), but it never progressed beyond a student project and then some guys at Google came along and wrote something much less good as clang-format.

Re:ahhhh advertising, my good friend!

[0100010001010011]

Only $5k? I wish I could buy development tools for my workflow for that cheap.

Re:ahhhh advertising, my good friend!

[avgjoe62]

This demonstrates the power of coffee.

Coffee, making people out of assholes since the 1600's...

Re:ahhhh advertising, my good friend!

[toonces33]

That's about how I look at it. Anyone can download the list of the things that the tool found and see for themselves what kinds of issues it was uncovering - the things that it is finding are certainly valid concerns.

What I find interesting about what it did find is that some of the issues are looking at the code formatting as an indication of what was intended, and it is flagging things that seem suspicious and/or inconsistent. Just using a normal compiler isn't going to catch these sorts of things since compilers generally don't care about whitespace.

Yeah, it is a lot of money. Really good developer tools usually are since you don't sell as many seats (Note that I haven't used this one, so I can't say whether it fits into the "really good" category). But finding these sorts of problems the "old-fashioned" way is generally a lot harder.

Sometimes one can find these sorts of things by turning compiler warning levels up. But not all such problems can be found this way.

Re:ahhhh advertising, my good friend!

[TheRaven64]

No, I use tabs for indentation. I use spaces for alignment. Every line starts with n tabs, where n is the current indent level. If I need to line things up, then I will use spaces after the tabs.

Re:ahhhh advertising, my good friend!

[DrXym]

Yes ordinarily I would use std:string and a bunch of useful classes from boost. But in this case it wouldn't have helped because a) I didn't write the code and if I had done I would have known to use the buffer length in the struct to control the copy and b) a std:string still has to be partially copied into a buffer so it is prone the same issue - something would have to copy the chars from the c_str() and that could trigger a problem.

Another point is that many of the so-called "safe" methods, e.g. strncpy_s are certainly safer from the point of view that they won't do anything if the dest cannot receive the src but they're less safe from another in that the apis are more complex and to use them properly you need to check for failure and handle that. So it makes the code more complex which adds its own risk of bugs.

Ideally stick with C++ classes but it's not possible when dealing with C APIs like Win32.

Re:ahhhh advertising, my good friend!

[rthille]

Looks like the FreeBSD team disagreed with you, as they modified the if test to bracket in the 2nd statement:

https://github.com/freebsd/fre...

Re:ahhhh advertising, my good friend!

[nullchar]

Pretty sure they will scan open source code for free, especially if easily accessible like on github.

Re:ahhhh advertising, my good friend!

[nullchar]

I used to do the same, but now I let it go. I just tab-indent everything and many times things don't line up visually, but I've grown used to it. Makes it easy to quickly format merged or other code snipits.

Re:ahhhh advertising, my good friend!

[arth1]

It says in TFA that FreeBSD already runs Coverity, but PVS-Studio found an additional 1000 bugs anyway

You're reading too much into it. It found around a thousand defects, but as far as I can tell, nothing was done to correlate those to the ten times as many defects that Coverity has marked as present and not yet fixed. Checking each and every defect against what Coverity found would be quite a lot of work.
It would surprise me very much if a majority of the 1000 weren't also already discovered by Coverity, and that any surplus that PVS-Studio found and Coverity didn't are mostly false positives.

Re:ahhhh advertising, my good friend!

[Puff_Of_Hot_Air]

The safe strcpy versions crash your program by default unless you go to a bunch of effort to handle the specialised exception in a different way. The basic idea is that buffer overruns are worse then crashing, so how about we crash straight away to reveal where the bug is. There is really no reason not convert all of your strcpy's etc to strcpy_s, you'll only crash when it would otherwise be a buffer overrun.

Re:ahhhh advertising, my good friend!

[stoatwblr]

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

I work in a space lab. Spending this kind of money is easily justified on the basis that projects are either long-lived (20+ years) and/or software updates are often difficult (stuff that goes in a spacecraft is hard to do field calls on), so making sure stuff is well written in the first instance saves more than that in the long term.

Perhaps PVS should offer their services to Toyota.

Re:ahhhh advertising, my good friend!

[JohnStock]

How are those TPS reports coming along? Yeahhhh.. Gonna have to ask you to come in on Saturday.. Mmmkay?

Poor Practices by PVS Studio and HexRays

[ilikenwf]

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.

Re:Poor Practices by PVS Studio and HexRays

[ThorGod]

It's pretty clearly just a marketing strategy if they're not giving teams access to their tools and their reports.

Re:Poor Practices by PVS Studio and HexRays

[ilikenwf]

Yeah, which sucks. People would be more interested if they'd at least provide the xml exports from their tools.

Not to mention licensing in a way that makes people able to afford and/or use the software for open source and free software - part of their analyzer uses clang, the least they could do is actually contribute toward that project and the ones that they "analyze."

Re:Poor Practices by PVS Studio and HexRays

[Andrey_Karpov]

Perhaps, I didn't get you quite right. Could it be that you didn't read the article attentively enough? The things we write about aren't really nonsense. For example, we are aware what is strcpy and memcpy. At times we see that people read our article not very thoroughly and then start arguing. I'll provide such an example concerning memcpy - see Fragment N1 and the comments below. This is a completely different case, I've provided it to show that the readers should carefully look though the code examples we provide.

Re:Poor Practices by PVS Studio and HexRays

[tlhIngan]

Yep, I've looked at the article and found a couple legitimate bugs, and the rest of it is the authors complete misunderstanding of what he's talking about. He doesn't seem to understand that strcpy and memcpy DO NOT DO THE SAME THING. He assumes that an extra tab means an if was done incorrectly, goes on about bad practices when its just that he doesn't know what the code is doing and taking 3 seconds to understand that a MACRO behaves differently on different architectures and maybe, just maybe, the hardcode 0 makes sense on that specific architecture and not on others ... which he could have found had he simply checked the places where the MACRO was defined instead of just the one that was compiled.

Or perhaps the trickiest bugs are caused by the most subtle of errors?

I mean, the concerns raise issues with what's happening in the code.

The tab issue - the code LOOKS like it does one thing, but it does something else completely. Depending on where it is in the code it could be on the scale of the "goto fail" bug (which was the result of the same kind of error) or it just leads to an obscure crash if certain conditions are right.

And when blocks of code in an if-then-else statement start being identical, that usually mean something is messed up. Or when you compare something against itself.

A lot of bugs are subtle ones - they don't fail immediately, but perhaps after a little while things start going wonky because the condition is being tripped. Or perhaps it's a security issue.

Perhaps it's being pedantic, but it looks like it catches what could be issues that would take weeks to debug.

And sometimes stuff is hidden through macros so it looks innocent, but because of the way the macro is written, it comes out completely different. Not parenthesizing macros leads to all sorts of strange bugs since the C preprocessor just works on blocks of and not semantics. So a macro that looks like a function doesn't necessarily behave like one (e.g., any math done in a macro is passed verbatim to the macro and not evaluated.

I'd say the bugs found are not the typical crash bugs or reproducible ones you fine, but more along the lines of those that happen infrequently and result in crashes that no one can really determine the cause because the code paths are executed frequently.

This ought to fix most of that ... :-)

[fahrbot-bot]

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

/*NOTREACHED*/

The tab thing

[Anonymous Coward]

No the tab thing, he's likely correct on.

if (something is there)
tab1 dinit the something
tab2 close the something

It does look like those two things are supposed to be executed in the if. The close presumably tests the handle and rejects it, so it doesn't fail, but it does need fixed.

On the macro thing, they pass in 10, or 0 to that macro and it ignores it and uses 0. But so what, thats just cleanup if you get time.

It's all very meh! Each change carries a risk, I've seen some of the most obscure bugs introduced by well meaning code cleanups!

Re:The tab thing

[arth1]

No the tab thing, he's likely correct on.

if (something is there)
tab1 dinit the something
tab2 close the something

It does look like those two things are supposed to be executed in the if.

Only if you don't have your tabs set correctly according to the standards for the project.

This is perfectly fine: /* ex:ts=4
*/
[4 spaces]if (something is there)
[8 spaces]tab1 dinit the something
[tab]tab2 close the something

It will display with the first and third lines aligned, and the second line indented.
However, if you attempt to read that in an editor that doesn't have modline support and by happenstance defaults to 8 characters fill for tab, it will display as if the second and third line are both indented. That's your fault.use editors in a different OS that your code doesn't support in the first place.

Granted, it is better to always transform tabs into spaces, but it's by no means required to cater to those who don't follow the standards you have set. Especially not if they're using a proprietary editor on an OS you don't even support.

Re:The tab thing

[ChunderDownunder]

Any sufficiently large and well organised programming team will have a style-guide and instructions for configuring your editor specific to the project. Style checkers will run as part of automated builds to determine non-compliant code, which can be handed to interns as busy-work to correct (with code reviews as per necessary). Developers will be requested to pretty-print their code against an indentation rules file prior to checking in work, which will be available as a command line tool or as an IDE function.

e.g. one of the first functions Theo and the libressl team did when creating the heart-bleed inspired fork was to reformat every source file according to OpenBSD guidelines.

That might sound like a team-lead has OCD and open a holy-war on tabs vs spaces but at the end of the day you accept the rules of the guys that are paying you... (Dunno how that's enforced on volunteer projects, mind you!)

Re:The tab thing

[fnj]

No the tab thing, he's likely correct on.

if (something is there)
tab1 dinit the something
tab2 close the something

It does look like those two things are supposed to be executed in the if.

Wrong. To any halfway competent C programmer it "looks like" no such thing. Because he knows that if there are no braces, the if statement always acts on the single following statement.

If it is C, not Python, the indentation means absolutely nothing except a visual cue, and any C programmer who relies only on visual cues is a C programmer of unacceptable quality.

Re:The tab thing

[Anonymous Coward]

To a competent C programmer, it's obvious what the code *does*.

It's not always obvious what it was *intended* to do. IOW, "is this a bug or not?"

Re:The tab thing

[metaforest]

if (something is there)
tab1 dinit the something
tab2 close the something

While the example is obvious to a competent C programmer, it is NOT obvious what was intended. And that is the very core of why coding style guides are used, and often heavy-handedly required by PMs.

if (condition) {
              statement1;
}
              statement2;

The deliberate braces (through technically redundant) make it clear what was intended even though statement2 has been indented improperly. I'd expect a static analyzer to flag either example as a style error. The reason is that the intent of the programmer is ambiguous. The reason it is ambiguous is that a botched copy-paste action, or a bad patch can cause formatting errors like this and then the intended meaning is in doubt.

if (condition) {
            statement1;
}
statement2;

or

if (condition) statement1;
statement2;

Is not going to get flagged, as all of the elements are stylistically coherent. Though, IMO, the second form is not good style, it is still clear what was intended.

Its size of code and users not dev team ...

[perpenso]

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

Why? One month of developer time is one month of developer time regardless of the size of the team. Either the product saves that much or more or it does not. If it does it is worthwhile.

As for whether a developer can afford the cost that again is not the function of the team size, rather the popularity of the dev team's product, the number of users. With a sufficiently sized market the revenue or donations would cover the cost regardless of the size of the team.

Re: Its size of code and users not dev team ...

[Entrope]

The software provider probably has costs beyond developer time: reputation, customer support, regulatory agencies, legal liability, and/or others. Escaped defects affect those, and the cost to avoid delivering buggy software should factor those savings in.

Two character comments are good ...

[perpenso]

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.

Re:Two character comments are good ...

[Rockoon]

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.

If you are documenting your intent then you are doing it wrong. As pointed out, code that is formated correctly (and consistently) should be the documentation of your intent. C and C++ are expressive enough that the source does not need to obfuscate the intent, and comments of the intent can disagree with the code so are really just unnecessary noise.

Comments should document the variables and their inter-relationships. This works both when you are writing bland straightforward code as well as when you are pulling from a grab bag of clever tricks in the name of optimization. It is these variable comments combined with the code (aka intent) that allows the programmer to see the logic of it all, and you better see the logic of it all if you intend to maintain the code.

Re:Two character comments are good ...

[AmiMoJo]

I never understood people who try to minimize the use of brackets. Do they also remove whitespace because it isn't needed, and who cares about readability anyway?

Re:Two character comments are good ...

[TheRaven64]

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.

Re:Two character comments are good ...

[perpenso]

If you are documenting your intent then you are doing it wrong.

The "two character comment" that I am referring to is a pair of parenthesis. The parenthesis are functional, they affect the parsing of expressions. They have a dual role of documenting intent and manifesting that intent in the code. Parenthesis, even when strictly unnecessary and just there for readability (a comment if you will), is "doing it correctly".

Bethesda Softworks

[Merovign]

Somebody get this to Bethesda, stat!

BitZtream was wrong. They just fixed it.

[Anonymous Coward]

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

Re:Lol they lead with goto-fail

[arth1]

> tabs are important.

They are important to avoid.

Says one who never writes makefiles or Fortran code.

Tabs are specified as whitespace in the C standard, and it's perfectly fine to use them. It may be better to always use spaces, unless space is an issue. If you use them, it's wise to also let viewers know what the max tab size is set to, like

/* ex:ts=4
      Project default tab size: 4
*/ ... rest of code ...

That way, copying and pasting between files that uses tabs and files that use spaces won't be much of a problem. Good editors should parse the ex modeline statements automatically, and if not, you can't say you weren't told.

he's way overconfident

[epine]

A big code fragment was copied, but later no changes were made.

Or perhaps during debugging, it was copied, experimental changes were made on one execution path (perhaps just a debug statement), then it was decided the changes weren't all that helpful, and the changes were deleted again, leaving both blocks identical (considered mostly harmless, but ought to have a comment if deliberately left that way).

Re:he's way overconfident

[ledow]

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.

Re:Lol they lead with goto-fail

[lgw]

Says one who never writes makefiles

Makefiles: the greatest argument ever made by mankind that tabs should be forever banned. (Or spaces, either way, but only one sort of leading whitespace should be syntactically legal in any given programming language.)

Re:Have they checked systemd?

[Megol]

This isn't the correct forum to publish your perverse sexual fantasies.

Re: "their" company

[Andrey_Karpov]

I am busy doing many things. I am one of the founders of PVS-Studio. Besides that, I also publish the news and communicate with the audience. I suppose, it's a nice thing that you can discuss technical moments with me.

Re:Have they checked systemd?

[Rockoon]

I for one would like to hear more of the things that he wishes systemd would do to him.

Oh, Karpov, you inveterate spammer...

[psychonaut]

The submitter, Andrey Karpov, is one of the developers of PVS-Studio. The article he's plugging was written by yet another PVS-Studio developer. I wouldn't be in the least surprised if this got voted to the front page by an army of PVS-Studio sockpuppets. They've been doing the same thing on Wikipedia for years (though their site was long ago put on a Wikimedia-wide spam blacklist), and also post similar spamvertisements, masquerading as "bug reports", to the issue trackers of prominent free software projects such as Mozilla.

Re:Oh, Karpov, you inveterate spammer...

[Andrey_Karpov]

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! :)

Re:Oh, Karpov, you inveterate spammer...

[psychonaut]

I had nothing to do with your websites getting blacklisted from Wikipedia. The administrators in the anti-spam brigade did that back in 2008.

Re:Oh, Karpov, you inveterate spammer...

[shutdown -p now]

You call it "spam", yet every single article from PVS that I've seen anywhere always points out actual code defects in real world projects.

Re:Lol they lead with goto-fail

[Viol8]

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.

Re:Another one?

[Andrey_Karpov]

Static analysis development is an opportunity for us to achieve some success in life. It isn't very interesing just to do outsource work, as we want something more. It feels that we are getting there. :)

Re:Have they checked systemd?

[bytesex]

No, this IS the correct forum to publish your perverse sexual fantasies - have you never read a GNAA post? Or that one about the poopeater?

Re:How the fuck are you so sure, paco?

[TheRaven64]

Deregistering something that was not registered properly in the first place is often a very dangerous, and incorrect, thing to be doing!

It is a bug, but it's not actually dangerous. The unregister function contains checks that the thing that it's unregistering is registered and silently does nothing if it isn't. If this weren't the case, lots of people would have seen odd behaviour.

Dear PVD Team:

[buddyglass]

Please do linux, glibc, openssl, MariaDB, PostgreSQL, httpd, nginx, Chrome, Firefox, python, ruby and gcc next. Thanks.

Re:Dear PVD Team:

[Andrey_Karpov]

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:

• Linux kernel
• Chrome (Chromium) 1, 2, 3, 4
• glibc
• OpenSSL, second
• PostgreSQL
• Firefox, second

Re:Dear PVD Team:

[Andrey_Karpov]

I really don't know. We just got several requests to check this project. The more people ask us to check something, the higher the chance that we'll do it. :)

Re:Dear PVD Team:

[buddyglass]

Awesome! Thanks for there response, and for the positive effect you're having. Fewer bugs makes me happy.

Re:Another one?

[Andrey_Karpov]

Well, it's not very likely that we'll be given a chance to run the analysis on Windows. Even if such a thing happens, we can't write an article about that. In general, we like checking Microsoft projects. These programs are of high quality and it's a big achievement for us to find something worthwhile, as well another opportunity to advertise PVS-Studio.

Here are the articles about our project checks:

• Computational Network Toolkit (CNTK) analysis: http://www.viva64.com/en/b/0372/
• ChakraCore: analysis of JavaScript-engine for Microsoft Edge: http://www.viva64.com/en/b/0370/
• CoreCLR analysis: http://www.viva64.com/en/b/0310/
• Windows 8 Driver Samples heck: http://www.viva64.com/en/b/0199/
• Amusing archaeology. Checking Microsoft Word 1.1a: http://www.viva64.com/en/b/0245/
• Also we have found bugs in Visual C++ libraries: http://www.viva64.com/en/b/0163/ , http://www.viva64.com/en/b/0288/
• However, we are not always able to write an article about errors after checking Microsoft projects. Microsoft is known for a reason for their responsible approach to creating software and qualitative code. For example in the Casablanca project I haven't found anything: http://www.viva64.com/en/b/0189/

Here are the checks of C# projetcs:

• Analysis of .NET Core Libraries (CoreFX): http://www.viva64.com/en/b/0365/
• Checking .NET Compiler Platform (Roslyn), on the occasion of the PVS-Studio release with C# support: http://www.viva64.com/en/b/0363/
• Analysis of Microsoft Code Contracts: http://www.viva64.com/en/b/0361/
• It's not our article, but on the same topic. Microsoft Orleans project check: http://shcherbak.me/post/139216516264/checking-microsoft-orleans-with-pvs-studio-for-fun

Re:Another one?

[110010001000]

There are lots of other projects to work on that will give you more success than static code analyzers. The market is saturated with those, and many of them are free.

Re:Lol they lead with goto-fail

[lgw]

Which is fine until you mix tabs and spaces to get that just-perfect indentation of some line, as always ends up happening. (Plus, your editor could set the width of leading spaces to whatever you wanted it to, if it actually mattered).

The "recurring check" warning isn't

[scdeimos]

Because these two blocks of code are not the same (spot the difference). Here is block 1:

static int
qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....)
{
....
if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) ||
(*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 2) != 10)) { // <=
return -1;
}
....
}

Here is block 2:

static int
qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....)
{
....
if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) ||
(*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 3) != 10)) {
return -1;
}
....
}

P.S.: Slashdot's comment editor sucks ass.

Re:This is getting to be quite a crowded space...

[arth1]

The mentioned in TFA that FreeBSD already uses Coverity.

Correct.

So they've only found things that Coverity missed.

That does not follow. There are over 10,000 defects which Coverity lists for the project which have not yet been fixed or marked as dismissed.

There is likely a substantial overlap between what PVS-Studio found and what Coverity found. Unless going through the results side-by-side, you won't know, but I think this is a reasonable assumption, given that the FreeBSD project doesn't have the resources to follow up on everything Coverity reports.

Re:Special

[kmoser]

This isn't 1980, we can parse identifier names that are longer than 2-3 characters.

You realize that the BSD project dates back to the 1970s, and Unix itself dates back to the 1960s?

Re:Have they checked systemd?

[Megol]

Now you made me feel sick :(