Large vulnerability report submitted to Firefox team

[Dominus Atheos]

Techspot

The Firefox team got more than just a handful of work handed to them recently when an investigating coder presented to them a list of 71 potential vulnerabilities and 655 “code defects” in the browser. Focusing on the 1.5.0.6 release of Firefox, analysis done by Klocwork's static analysis tool doesn't necessarily mean action will be taken or even required, just that something might be wrong. While not releasing specific details, they did give the public a general idea of what was found:

By far, the majority of the defects reported were null pointer dereferences (446 defects). A large number of defects resulted from the code not checking for null after memory was allocated. In addition, there were many cases where the return value of functions designed to return null were not checked prior to dereferencing.

Some might see this as a strike against Firefox, but in fact it is exactly the opposite. Unlike a closed source browser such as IE, having the source available to the public makes this kind of exploration possible and may in fact encourage improvement. There's no word from the Firefox team just yet, though I'm sure they will look into the findings.

[Arrow]

WTF? Not checking for null after allocation and at release? That's programming 101!

[TheMuffinKing]

Last time I checked Firefox was free. This stuff happens when there is no quality control, or any monetary incentive. I'm sure they'll fix it and I still like it better than Internet Explorer.

[General Zod]

Last time I checked Firefox was free. This stuff happens when there is no quality control, or any monetary incentive. I'm sure they'll fix it and I still like it better than Internet Explorer.

Firefox does receive funding from various sources, however. So saying there is no monetary incentive is factually incorrect. Considering that it's staffed by a variety of members, saying there's no quality control either is frankly idiotic.

[Sharp-kun]

Some might see this as a strike against Firefox, but in fact it is exactly the opposite. Unlike a closed source browser such as IE, having the source available to the public makes this kind of exploration possible and may in fact encourage improvement. There's no word from the Firefox team just yet, though I'm sure they will look into the findings.

[Dooey Jo]

WTF? Not checking for null after allocation and at release? That's programming 101!

Maybe C 101, but AFAIK Firefox is written in C++, which handles such things automagically. Now, I haven't checked the source code (maybe they mix C and C++), but if all of those bugs are of the type "delete p" where p might be NULL, then they are non-issues. It's the preferred way of doing it in C++; checking for NULL is redundant.

[Arrow]

Maybe C 101, but AFAIK Firefox is written in C++, which handles such things automagically. Now, I haven't checked the source code (maybe they mix C and C++), but if all of those bugs are of the type "delete p" where p might be NULL, then they are non-issues. It's the preferred way of doing it in C++; checking for NULL is redundant.

And yet its still a good programming practice. Never assume malloc() (and its varients) or new [] are going to give you memory - I've seen both crap out (new crapped on my coworker last week, for instance). And while delete will tolerate a NULL, free() won't, and its wrong to assume any method/function you call will check for NULL parameters (unless you wrote it, or unless its provide by a vendor you trust with documentation the states it makes the check).

Maybe I'm being more stricted than I have to be, but I work in a mixed C and C++ environment, and shit like this has cost me time before.

[Dooey Jo]

And yet its still a good programming practice.

I disagree. It seems more like checking that a STL vector isn't empty before you clear it, just in case someone's library doesn't comply to standards. Maybe it will save someone a few bugs sometime, but really, he would be better off by getting a new library anyway (who knows what other non-standard things it might do). Or a new compiler as it would be in this case.

Never assume malloc() (and its varients) or new [] are going to give you memory - I've seen both crap out (new crapped on my coworker last week, for instance). And while delete will tolerate a NULL, free() won't, and its wrong to assume any method/function you call will check for NULL parameters (unless you wrote it, or unless its provide by a vendor you trust with documentation the states it makes the check).

new and new [] will throw exceptions if allocation fails, but it appears Visual Studio (at least older versions) doesn't support that fully. As for free(), according to ANSI C, it too should accept a NULL. But even if it doesn't, it's extremely bad practice to use delete interchangeably with free() anyway, since they are hardly the same things. But again, delete NULL is guaratanteed to do nothing, so if that's the majority of the "bugs", then they really are nothing but proper C++.

[Arrow]

I disagree. It seems more like checking that a STL vector isn't empty before you clear it, just in case someone's library doesn't comply to standards. Maybe it will save someone a few bugs sometime, but really, he would be better off by getting a new library anyway (who knows what other non-standard things it might do). Or a new compiler as it would be in this case.

And that's an apples-to-oranges comparison. The one of the major points of using a vector over an array is that you know its going to manage memory for you, instead of you having to manage the memory yourself. It also falls under one of those libraries you can trust (generally).

new and new [] will throw exceptions if allocation fails, but it appears Visual Studio (at least older versions) doesn't support that fully. As for free(), according to ANSI C, it too should accept a NULL. But even if it doesn't, it's extremely bad practice to use delete interchangeably with free() anyway, since they are hardly the same things. But again, delete NULL is guaratanteed to do nothing, so if that's the majority of the "bugs", then they really are nothing but proper C++.

And older versions of Visual Studio also don't have free checking for null. I haven't tried it on VS 8 yet.

"Should" does not always translate into "it does". And I'll point you back to Destructionator's post about their coding practices, because they "should" be checking nulls.

[Dooey Jo]

And that's an apples-to-oranges comparison. The one of the major points of using a vector over an array is that you know its going to manage memory for you, instead of you having to manage the memory yourself. It also falls under one of those libraries you can trust (generally).

You can trust someone's implementation of a library recommendation, but not their implementation of the actual language? That seems a bit paranoid to me.

And older versions of Visual Studio also don't have free checking for null. I haven't tried it on VS 8 yet.

Perhaps not, but again, free() is part of C, and if you're using C++, there's no reason to use it over new, unless you for some reason want to go really low-level. Maybe they do use it in Firefox, so that NULL pointers might pose a problem on certain compilers, but I don't know.

"Should" does not always translate into "it does". And I'll point you back to Destructionator's post about their coding practices, because they "should" be checking nulls.

Yes, after malloc(), because it will not throw an exception. If they want to check for NULL after a new [] then they can do that of course, but unless they have exceptions disabled I don't really see the point. It'll just make the code ugly...

[Arrow]

You can trust someone's implementation of a library recommendation, but not their implementation of the actual language? That seems a bit paranoid to me.

Maybe it is, but I've been using Visual Studio since 6.0, and I've seen plenty of weird, buggy shit, especially when it comes to C++ and STL (VS 6 STL support was an abortion, and IIRC, required a third party patch to get everything working correctly). And IIRC, in 6.0, new didn't throw an exception (but that years ago, so I could be wrong).

Perhaps not, but again, free() is part of C, and if you're using C++, there's no reason to use it over new, unless you for some reason want to go really low-level. Maybe they do use it in Firefox, so that NULL pointers might pose a problem on certain compilers, but I don't know.

I know programmers, especially old-schoolers, that still prefer the old C calls for allocating in C++. They also hate exceptions, because they uglify the code (and I agree with them on that). Maybe the Firefox guys are like that, and like you said, we don't know.

Edit: Also, let me say that a bit of my "paranoria" comes from using other vendors stuff saying it does one thing when actually does another, including how it allocates memory for DMA.