Chromium: Typos

We present a series of articles where we share some tips on how to write high-quality code, using the bugs found in the Chromium project as examples. This is Part 4, where I talk about the problem of typos and writing code using the "Copy-Paste method".

Nobody is safe from typos. They are found even in the code written by the most skilled programmers. Sadly, skill and experience don't protect you from accidentally writing a wrong variable name or omitting a few symbols. That's why typos have always been around, and always will be.

Writing code using the Copy-Paste method adds to the number of defects in source code. Unfortunately, it is a very effective technique, and there's nothing to be done about that. It takes much less time to copy a few lines and make slight changes in them than type the entire block of code from scratch. I'm not even going to bother trying to talk you out of using this technique, as I'm guilty of using it too. The annoying side effect of Copy-Paste is that you could easily forget to modify the code copied. Such mistakes are also sort of typos, as they have to do with forgetting to change the copied block of code or making wrong changes as a result of inattention.

Let's look at the typos that I found when examining the analysis report generated by PVS-Studio. As I mentioned in the introductory article, I only skimmed through it, so I may have missed some defects. In this article, I will be talking about the silliest and simplest mistakes, but that doesn't make any one of them any less of a mistake.

Null-pointer Dereference

The Common Weakness Enumeration classifies null-pointer dereference as CWE-476.

The examples below are taken from the source code of the Chromium project.

The inline_style pointer is dereferenced before being checked for nullptr. I guess this is because of a typo: the programmer simply forgot to add the asterisk character '*', in which case the correct version should look like this:

However, it could be the inline_style pointer that the programmer actually wanted to check. In that case, we're dealing with an error in function logic rather than a plain typo. To fix it, the check must be moved up so that it is executed prior to the call to the GetStylesForUIElement function. The function should then look like this:

The object pointer is first dereferenced and only then checked for nullptr. Well, the entire expression looks odd, as if it was written in haste, which caused the typo: the programmer first wrote object->IsSmi(), then recalled that the object pointer should be checked for nullptr, and added the check. What they didn't do is pause and think it all over :).

V713 CWE-476 The pointer object was utilized in the logical expression before it was verified against nullptr in the same logical expression. ic-inl.h 44

Copy-Paste

Errors that stem from the use of Copy-Paste can't be classified under the Common Weakness Enumeration: there's simply no such defect as "Copy-Paste" :). Different typos cause different defects. The bugs I'm discussing below fall under the following categories:

I can almost sense the programmer's laziness and reluctance to retype the variable name signin_scoped_device_id. So, they decided to copy it. However, together with the name, they accidentally copied the std::string type:

std::string signin_scoped_device_id

The result of that laziness is that the value returned by the GetSigninScopedDeviceId function will be stored to a temporary variable, which will be destroyed right after that.

The programmer must have copied StandardFrameConstants::kCallerPCOffset intending to change the constant's name but forgot to do so. As a result, the constant is subtracted from itself, resulting in 0, which is then divided by the value of kPointerSize. But that doesn't matter anymore since the result will be 0 anyway.

PVS-Studio diagnostic message: V501 There are identical sub-expressions 'StandardFrameConstants::kCallerPCOffset' to the left and to the right of the '-' operator. linkage.h 66

Whatever the condition, the program performs the same actions. This must be the result of poorly done Copy-Paste. The programmer copied a block of code, got distracted, and forgot to change the else-branch.

The same check is performed twice. Either the second check is unnecessary or something else was to be checked. The analyzer reports this suspicious code with two warnings at once:

V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 758, 761. skpathref.cpp 761

V649 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 758, 761. skpathref.cpp 761

PVS-Studio diagnostic message: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'res_y' variable should be used instead of 'res_x'. cfx_imagetransformer.cpp 201

The programmer copied the line:

if (*res_x < 0 && *res_x > -kBase)

and changed one instance of the variable name res_x to res_y but forgot about the second. As a result, the function lacks the *res_y > -kBase check.

Other Typos

While it was easy to classify typos into the "null-pointer dereference" and "Copy-Paste" categories, the rest bugs are quite diverse. I just put them all into this section rather than trying to categorize them.

What comes first is a code snippet from Chromium. I'm not sure if this is an error, but the developers surely need to check it out.

Because of the typo, the u object is compared with itself. It turns out that operator == treats any two objects as identical.

PVS-Studio diagnostic message: V549 CWE-688 The first argument of 'memcmp' function is equal to the second argument. skpdfcanon.h 67

The next typo - in the same library - has to do with a function that outputs information about the first element of an array rather than print out all of its elements. However, it's not that bad because the function is used for debugging.

Two op == are missing. As a result, the condition includes the constants Ice::InstArithmetic::Lshr and Ice::InstArithmetic::Ashr, which are not compared with any value. This is obviously an error which makes these two expressions always true.

V768 CWE-571 The enumeration constant 'Lshr' is used as a variable of a Boolean-type. subzeroreactor.cpp 712

V768 CWE-571 The enumeration constant 'Ashr' is used as a variable of a Boolean-type. subzeroreactor.cpp 712

By the way, I also found a few code fragments that are not bugs or typos by themselves, but they do pave the way for typos in the future. Such are, for example, poorly chosen names of global variables.

One such variable can be found in the Yasm library:

static int i; /* The t_type of tokval */

PVS-Studio diagnostic message: V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'i' variable. nasm-eval.c 29

Yes, it's not an error yet. But you may well forget to declare a local variable i at some point and use the global one instead. Yes, the code would compile, but no one could tell in what way the program would be affected. So, the moral is: choose more specific names for your global variables.

I hope I have managed to convince you that bugs caused by typos could be very bad!

Rounding it off, I recommend reading about one nice typo in the Protocol buffers library, which I described in the separate post "February 31".

Recommendations

Sorry, no recommendations this time. There's no universal, clearly defined advice to give on how to avoid bugs discussed in this article. Human is just prone to errors - that's it.

However, I'll try and give you a few tips and hope they will help you make fewer typos.

Arrange complex conditions in the "table form". I described this technique in detail in my mini-book "The Ultimate Question of Programming, Refactoring, and Everything". Head to Tip 13 - Table-style formatting. By the way, I plan on writing an extended version of that book, with 50 tips instead of 42. Besides, some early chapters need update and refinement.

When using the Copy-Paste method, be especially careful when going through the final lines of the copied block. This has to do with the "last line effect", when programmers, writing similar blocks of code, relax and start making mistakes. If you want more scientific reading on the subject, refer to the article "The last line effect explained".

Be careful when writing functions that compare objects. Such functions can look misleadingly simple, which opens the way for a lot of typos. For details, see "The Evil within the Comparison Functions".

Mistakes are harder to notice in code that's hard to read. Keeping that in mind, try to write as neatly and clearly as possible. Unfortunately, this recommendation can't be formalized, especially in brief form. There are a lot of wonderful books on this topic such as "Code Complete" by Steve McConnell. As to the practical side, I recommend paying attention to your company's coding standard and enlarging it with every new useful technique that you come across. After all, C++ is rapidly evolving, so it makes sense to regularly audit and update the standard.

Use the PVS-Studio static analyzer regularly. After all, all the bugs you've read about in this article were found with PVS-Studio. Follow this link to download and try the analyzer.

Thanks for reading - but that's not all. Another article is coming soon.

Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

We use cookies for the analysis of events to improve our content and make user interaction more convenient. By continuing the view of our web-pages you accept the terms of using these files. You can find out more about cookie-files and privacy policy or close the notification, by clicking on the button.
Learn More →