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.
The Common Weakness Enumeration classifies null-pointer dereference as CWE-476.
The examples below are taken from the source code of the Chromium project.
class Display : ....
{
....
std::unique_ptr<FocusController> focus_controller_;
....
}
Display::~Display() {
....
if (!focus_controller_) {
focus_controller_->RemoveObserver(this);
focus_controller_.reset();
}
....
}The condition is incorrect: the pointer is dereferenced if null. The '!' character obviously shouldn't be here.
PVS-Studio diagnostic message: V522 CWE-476 Dereferencing of the null pointer 'focus_controller_' might take place. display.cc 52
The next error should be given the title "The classic of classics".
void AppViewGuest::CreateWebContents(....) {
....
if (!guest_extension ||
!guest_extension->is_platform_app() ||
!embedder_extension |
!embedder_extension->is_platform_app()) {
callback.Run(nullptr);
return;
}
....
}There's a typo here. The programmer accidentally wrote the '|' operator instead of '||'. As a result, the embedder_extension pointer is dereferenced, no matter if it's null or not.
Note. If you are only slightly familiar with the C++ language, I recommend reading the article "Short-circuit evaluation" to figure out why this is an error.
PVS-Studio diagnostic message: V522 CWE-476 Dereferencing of the null pointer 'embedder_extension' might take place. Check the bitwise operation. app_view_guest.cc 186
The next defect has to do with incomplete code. I think we should treat it as a typo too. The programmer intended to assign some value to a smart pointer but forgot to do so.
std::unique_ptr<base::ListValue>
NetworkingPrivateServiceClient::GetEnabledNetworkTypes() {
std::unique_ptr<base::ListValue> network_list;
network_list->AppendString(::onc::network_type::kWiFi);
return network_list;
}The smart pointer is null by default. Since it doesn't change after declaration, we get null-pointer dereference.
PVS-Studio diagnostic message: V522 CWE-476 Dereferencing of the null pointer 'network_list' might take place. networking_private_service_client.cc 351
Now let's find a more complex case.
Response CSSAgent::getMatchedStylesForNode(int node_id,
Maybe<CSS::CSSStyle>* inline_style)
{
UIElement* ui_element = dom_agent_->GetElementFromNodeId(node_id);
*inline_style = GetStylesForUIElement(ui_element);
if (!inline_style)
return NodeNotFoundError(node_id);
return Response::OK();
}PVS-Studio diagnostic message: V595 CWE-476 The 'inline_style' pointer was utilized before it was verified against nullptr. Check lines: 142, 143. css_agent.cc 142
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:
*inline_style = GetStylesForUIElement(ui_element);
if (!*inline_style)
return NodeNotFoundError(node_id);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:
Response CSSAgent::getMatchedStylesForNode(int node_id,
Maybe<CSS::CSSStyle>* inline_style)
{
UIElement* ui_element = dom_agent_->GetElementFromNodeId(node_id);
if (!inline_style)
return NodeNotFoundError(node_id);
*inline_style = GetStylesForUIElement(ui_element);
return Response::OK();
}That's all for null-pointer dereference errors in the code of Chromium, but there's one found in the V8 engine.
bool Object::IsSmi() const { return HAS_SMI_TAG(this); }
bool IC::IsHandler(Object* object) {
return (object->IsSmi() && (object != nullptr)) ||
object->IsDataHandler() ||
object->IsWeakCell() ||
object->IsCode();
}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 :).
Here PVS-Studio emits two warnings at once:
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:
Again, we'll start with those from the code of the Chromium project itself.
void ProfileSyncService::OnEngineInitialized(....)
{
....
std::string signin_scoped_device_id;
if (IsLocalSyncEnabled()) {
signin_scoped_device_id = "local_device";
} else {
SigninClient* signin_client = ....;
DCHECK(signin_client);
std::string signin_scoped_device_id = // <=
signin_client->GetSigninScopedDeviceId();
}
....
}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_idThe 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.
PVS-Studio diagnostic message: V561 CWE-563 It's probably better to assign value to 'signin_scoped_device_id' variable than to declare it anew. Previous declaration: profile_sync_service.cc, line 900. profile_sync_service.cc 906
The next error was found in the V8 engine employed by Chromium.
static LinkageLocation ForSavedCallerReturnAddress() {
return ForCalleeFrameSlot(
(StandardFrameConstants::kCallerPCOffset -
StandardFrameConstants::kCallerPCOffset) /
kPointerSize,
MachineType::Pointer());
}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
Here's another suspicious snippet from V8:
void JSObject::JSObjectShortPrint(StringStream* accumulator) {
....
accumulator->Add(global_object ? "<RemoteObject>" :
"<RemoteObject>");
....
}PVS-Studio diagnostic message: V583 CWE-783 The '?:' operator, regardless of its conditional expression, always returns one and the same value: "<RemoteObject>". objects.cc 2993
Now let's look at the PDFium project.
inline bool FXSYS_iswalpha(wchar_t wch) {
return FXSYS_isupper(wch) || FXSYS_islower(wch);
}
bool CPDF_TextPage::IsHyphen(wchar_t curChar) const {
WideStringView curText = m_TempTextBuf.AsStringView();
....
auto iter = curText.rbegin();
....
if ((iter + 1) != curText.rend()) {
iter++;
if (FXSYS_iswalpha(*iter) && FXSYS_iswalpha(*iter)) // <=
return true;
}
....
}The programmer copied FXSYS_iswalpha(*iter) and... And forgot to modify the second part of the condition.
PVS-Studio diagnostic message: V501 CWE-571 There are identical sub-expressions 'FXSYS_iswalpha(* iter)' to the left and to the right of the '&&' operator. cpdf_textpage.cpp 1218
A similar mistake in an expression can be found in the Protocol buffers library.
bool IsMap(const google::protobuf::Field& field,
const google::protobuf::Type& type) {
return
field.cardinality() ==
google::protobuf::Field_Cardinality_CARDINALITY_REPEATED
&&
(GetBoolOptionOrDefault(type.options(), "map_entry", false) ||
GetBoolOptionOrDefault(type.options(),
"google.protobuf.MessageOptions.map_entry", false) || // <=
GetBoolOptionOrDefault(type.options(),
"google.protobuf.MessageOptions.map_entry", false)); // <=
}The code was written using Copy-Paste - that's for sure. No one would ever retype such a long line :).
PVS-Studio diagnostic message: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. utility.cc 351
By the way, there's another similar error nearby: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. utility.cc 360
The next code fragment, taken from the SwiftShader library, demonstrates my favorite "last line effect". What a nice bug! That's the kind of bugs I like.
void TextureCubeMap::updateBorders(int level)
{
egl::Image *posX = image[CubeFaceIndex(..._POSITIVE_X)][level];
egl::Image *negX = image[CubeFaceIndex(..._NEGATIVE_X)][level];
egl::Image *posY = image[CubeFaceIndex(..._POSITIVE_Y)][level];
egl::Image *negY = image[CubeFaceIndex(..._NEGATIVE_Y)][level];
egl::Image *posZ = image[CubeFaceIndex(..._POSITIVE_Z)][level];
egl::Image *negZ = image[CubeFaceIndex(..._NEGATIVE_Z)][level];
....
if(!posX->hasDirtyContents() ||
!posY->hasDirtyContents() ||
!posZ->hasDirtyContents() ||
!negX->hasDirtyContents() ||
!negY->hasDirtyContents() || // <=
!negY->hasDirtyContents()) // <=
{
return;
}
....
}What should have been used at the very end of the condition is the pointer negZ, not negY. This line was obviously written using Copy-Paste, with the programmer's forgetting all about it eventually.
PVS-Studio diagnostic message: V501 CWE-570 There are identical sub-expressions '!negY->hasDirtyContents()' to the left and to the right of the '||' operator. texture.cpp 1268
The WebKit engine has a nice bug too:
bool IsValid(....) const final {
OptionalRotation inherited_rotation =
GetRotation(*state.ParentStyle());
if (inherited_rotation_.IsNone() ||
inherited_rotation.IsNone())
return inherited_rotation.IsNone() ==
inherited_rotation.IsNone();
....
}PVS-Studio diagnostic message: V501 CWE-571 There are identical sub-expressions 'inherited_rotation.IsNone()' to the left and to the right of the '==' operator. cssrotateinterpolationtype.cpp 166
The programmer copied the inherited_rotation.IsNone() call and forgot to add the underline character, '_'. Correct version:
return inherited_rotation_.IsNone() ==
inherited_rotation.IsNone();Let's peek into the Protocol buffers library again.
void SetPrimitiveVariables(....,
std::map<string, string>* variables) {
....
(*variables)["set_has_field_bit_message"] = "";
(*variables)["set_has_field_bit_message"] = "";
(*variables)["clear_has_field_bit_message"] = "";
....
}No comments needed. It's Copy-Paste, pure and simple. PVS-Studio diagnostic message: V519 CWE-563 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 149, 150. java_primitive_field_lite.cc 150
On we go. You all should know how treacherous Copy-Paste can be. Read and fear! The function below is taken from WebRTC.
size_t WebRtcSpl_FilterAR(....)
{
....
for (i = 0; i < state_length - x_length; i++)
{
state[i] = state[i + x_length];
state_low[i] = state_low[i + x_length];
}
for (i = 0; i < x_length; i++)
{
state[state_length - x_length + i] = filtered[i];
state[state_length - x_length + i] = filtered_low[i]; // <=
}
....
}Yes, it's them again - the implications of Copy-Paste. The programmer copied the following line:
state[state_length - x_length + i] = filtered[i];and changed filtered to filtered_low but forgot to change state to state_low. As a result, some of the elements of the state_low array remain uninitialized.
Tired of reading? Now you know how I feel writing this! Why don't we have a coffee break?
COFFEE BREAK.
Hope you feel refreshed and ready to go on enjoying the 50 shades of Copy-Paste. Here's what I found in the PDFium library.
bool CPWL_EditImpl::Backspace(bool bAddUndo, bool bPaint) {
....
if (m_wpCaret.nSecIndex != m_wpOldCaret.nSecIndex) {
AddEditUndoItem(pdfium::MakeUnique<CFXEU_Backspace>(
this, m_wpOldCaret, m_wpCaret, word.Word, word.nCharset));
} else {
AddEditUndoItem(pdfium::MakeUnique<CFXEU_Backspace>(
this, m_wpOldCaret, m_wpCaret, word.Word, word.nCharset));
}
....
}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.
PVS-Studio diagnostic messages: V523 CWE-691 The 'then' statement is equivalent to the 'else' statement. cpwl_edit_impl.cpp 1580
Three more issues of the same type. I'll spare you the details and cite only the warnings:
The Skia library.
bool SkPathRef::isValid() const {
....
if (nullptr == fPoints && 0 != fFreeSpace) {
return false;
}
if (nullptr == fPoints && 0 != fFreeSpace) {
return false;
}
....
}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:
Here's another error from the Skia library.
static inline bool can_blit_framebuffer_for_copy_surface(
const GrSurface* dst, GrSurfaceOrigin dstOrigin,
const GrSurface* src, GrSurfaceOrigin srcOrigin, ....)
{
....
const GrGLTexture* dstTex =
static_cast<const GrGLTexture*>(dst->asTexture());
const GrGLTexture* srcTex =
static_cast<const GrGLTexture*>(dst->asTexture()); // <=
const GrRenderTarget* dstRT = dst->asRenderTarget();
const GrRenderTarget* srcRT = src->asRenderTarget();
if (dstTex && dstTex->target() != GR_GL_TEXTURE_2D) {
return false;
}
if (srcTex && srcTex->target() != GR_GL_TEXTURE_2D) {
return false;
}
....
}PVS-Studio diagnostic message: V656 Variables 'dstTex', 'srcTex' are initialized through the call to the same function. It's probably an error or un-optimized code. Check lines: 3312, 3313. grglgpu.cpp 3313
The programmer forgot to replace dst with src after Copy-Paste. Correct version:
const GrGLTexture* srcTex =
static_cast<const GrGLTexture*>(src->asTexture());The HarfBuzz library.
inline int get_kerning (hb_codepoint_t left,
hb_codepoint_t right,
const char *end) const
{
unsigned int l = (this+leftClassTable).get_class (left);
unsigned int r = (this+leftClassTable).get_class (left); // <=
unsigned int offset = l * rowWidth + r * sizeof (FWORD);
....
}PVS-Studio diagnostic message: V751 Parameter 'right' is not used inside function body. hb-ot-kern-table.hh 115
Another nice error. The programmer copied the line but forgot to change two left's to right's. Correct version:
unsigned int l = (this+leftClassTable).get_class (left);
unsigned int r = (this+rightClassTable).get_class (right);The SwiftShader library. I'm sure these similarly looking blocks of code were written using Copy-Paste:
class ELFObjectWriter {
....
ELFStringTableSection *ShStrTab;
ELFSymbolTableSection *SymTab;
ELFStringTableSection *StrTab;
....
};
void ELFObjectWriter::assignSectionNumbersInfo(
SectionList &AllSections)
{
....
ShStrTab->setNumber(CurSectionNumber++);
ShStrTab->setNameStrIndex(ShStrTab->getIndex(ShStrTab->getName()));
AllSections.push_back(ShStrTab);
SymTab->setNumber(CurSectionNumber++);
SymTab->setNameStrIndex(ShStrTab->getIndex(SymTab->getName()));
AllSections.push_back(SymTab);
StrTab->setNumber(CurSectionNumber++);
StrTab->setNameStrIndex(ShStrTab->getIndex(StrTab->getName()));
AllSections.push_back(StrTab);
....
}The programmer was inattentive: they forgot to replace hStrTab->getIndex with SymTab->getIndex in the second block, and hStrTab->getIndex with StrTab->getIndex in the third.
PVS-Studio diagnostic message: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'SymTab' variable should be used instead of 'ShStrTab'. iceelfobjectwriter.cpp 194
The next error deals with incorrect rectangle-size calculation in the WebKit library. This code is a real eye strain. I bet you won't be able to spot the bug.
void NGFragmentBuilder::ComputeInlineContainerFragments(....)
{
....
value.start_fragment_union_rect.size.width =
std::max(descendant.offset_to_container_box.left +
descendant.fragment->Size().width -
value.start_fragment_union_rect.offset.left,
value.start_fragment_union_rect.size.width);
value.start_fragment_union_rect.size.height =
std::max(descendant.offset_to_container_box.top +
descendant.fragment->Size().height -
value.start_fragment_union_rect.offset.top,
value.start_fragment_union_rect.size.width);
....
}At the very end of the copied block, width should have been replaced with height.
PVS-Studio diagnostic message: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'height' variable should be used instead of 'width'. ng_fragment_builder.cc 326
Phew... We're almost done. The last fragment in this section is taken from the PDFium library.
void sycc420_to_rgb(opj_image_t* img) {
....
opj_image_data_free(img->comps[0].data);
opj_image_data_free(img->comps[1].data);
opj_image_data_free(img->comps[2].data);
img->comps[0].data = d0;
img->comps[1].data = d1;
img->comps[2].data = d2;
img->comps[1].w = yw; // 1
img->comps[1].h = yh; // 1
img->comps[2].w = yw; // 1
img->comps[2].h = yh; // 1
img->comps[1].w = yw; // 2
img->comps[1].h = yh; // 2
img->comps[2].w = yw; // 2
img->comps[2].h = yh; // 2
img->comps[1].dx = img->comps[0].dx;
img->comps[2].dx = img->comps[0].dx;
img->comps[1].dy = img->comps[0].dy;
img->comps[2].dy = img->comps[0].dy;
}A duplicate block of assignments. PVS-Studio diagnostic message: V760 Two identical blocks of text were found. The second block begins from line 420. fx_codec_jpx_opj.cpp 416
Oops, sorry, it's not over yet. Here's one more Copy-Paste from PDFium. I had to add it too.
void Transform(int x, int y, int* x1,
int* y1, int* res_x, int* res_y) const
{
....
if (*res_x < 0 && *res_x > -kBase)
*res_x = kBase + *res_x;
if (*res_y < 0 && *res_x > -kBase)
*res_y = kBase + *res_y;
}
}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.
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.
namespace cellular_apn {
const char kAccessPointName[] = "AccessPointName";
const char kName[] = "Name";
const char kUsername[] = "Username";
const char kPassword[] = "Password";
const char kLocalizedName[] = "LocalizedName";
const char kLanguage[] = "LocalizedName";
}The suspicious thing about it is that the constants kLocalizedName and kLanguage contain the same string. My guess is that the code should actually look like this:
const char kLanguage[] = "Language";But that's not for sure.
Here PVS-Studio issues the warning: V691 Empirical analysis. It is possible that a typo is present inside the string literal: "LocalizedName". The 'Localized' word is suspicious. onc_constants.cc 162
The next bug, found in the Skia library, is a real gem and refers us to the article "The Evil within the Comparison Functions".
inline bool operator==(const SkPDFCanon::BitmapGlyphKey& u,
const SkPDFCanon::BitmapGlyphKey& v) {
return memcmp(&u, &u, sizeof(SkPDFCanon::BitmapGlyphKey)) == 0;
}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.
SkString dumpInfo() const override {
SkString str;
str.appendf("# combined: %d\n", fRects.count());
for (int i = 0; i < fRects.count(); ++i) {
const RectInfo& geo = fRects[0];
str.appendf("%d: Color: 0x%08x, "
"Rect [L: %.2f, T: %.2f, R: %.2f, B: %.2f]\n", i,
geo.fColor, geo.fRect.fLeft, geo.fRect.fTop,
geo.fRect.fRight, geo.fRect.fBottom);
}
str += fHelper.dumpInfo();
str += INHERITED::dumpInfo();
return str;
}fRects[i] should be written in the place of fRects[0]. PVS-Studio diagnostic message: V767 Suspicious access to element of 'fRects' array by a constant index inside a loop. grnonaafillrectop.cpp 276
Because of a typo in the SwiftShader project, the assert macro fails to check some of the arguments.
static Value *createArithmetic(Ice::InstArithmetic::OpKind op,
Value *lhs, Value *rhs)
{
assert(lhs->getType() == rhs->getType() ||
(llvm::isa<Ice::Constant>(rhs) &&
(op == Ice::InstArithmetic::Shl ||
Ice::InstArithmetic::Lshr ||
Ice::InstArithmetic::Ashr)));
....
}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.
What the condition should actually look like is this:
assert(lhs->getType() == rhs->getType() ||
(llvm::isa<Ice::Constant>(rhs) &&
(op == Ice::InstArithmetic::Shl ||
op == Ice::InstArithmetic::Lshr ||
op == Ice::InstArithmetic::Ashr)));PVS-Studio issues two warnings here:
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".
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.
Thanks for reading - but that's not all. Another article is coming soon.