To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Starting My Collection of Bugs Found in…

Starting My Collection of Bugs Found in Copy Functions

Apr 04 2020
Author:

I've already noticed a few times before that programmers seem to tend to make mistakes in simple copy functions. Writing a profound article on this topic is going to take quite a while since I'll have to do some thorough research and sample collecting, but for now I'd like to share a couple of examples I stumbled upon recently.

0723_copy_functions/image1.png

The Baader-Meinhof phenomenon? I don't think so

As a member of the PVS-Studio team, I come across numerous bugs found with our tool in various projects. And as a DevRel, I love telling people about that :). Today I'm going to talk about incorrectly implemented copy functions.

I saw such functions before, but I never wrote them down as I didn't think they were worth mentioning. But since I discovered the tendency, I can't but start collecting them. For a start, I'm going to show you two recently found specimens.

You may argue that two cases don't make a tendency yet; that I paid attention solely because they occurred too close in time and the Baader-Meinhof phenomenon kicked in.

The Baader-Meinhof phenomenon, also called Frequency Illusion, is a cognitive bias where a person stumbles upon a piece of information and soon afterwards encounters the same subject again, which makes them believe this subject appears exceptionally frequently.

I don't think that's the case. I already had a similar experience with poorly written comparison functions, and my observation was later proved by real examples: "The Evil within the Comparison Functions".

Okay, let's get to the point. That introduction was a bit too long for a brief note on two examples :).

Example 1

In the article about the check of the Zephyr RTOS, I mentioned a failed attempt to make a function that should work like strdup:

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    ((u8_t *)mntpt)[strlen(mntpt)] = '\0';
    memcpy(cpy_mntpt, mntpt, strlen(mntpt));
  }
  return cpy_mntpt;
}

PVS-Studio diagnostic message: V575 [CWE-628] The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. shell.c 427

The analyzer says the memcpy function copies the string but fails to copy the terminating null character, which is a very strange behavior. You may think the copying of the terminating null takes place in the following line:

((u8_t *)mntpt)[strlen(mntpt)] = '\0';

But that's wrong – this is a typo that causes the terminating null to get copied into itself. Notice that the target array is mntpt, not cpy_mntpt. As a result, the mntpt_prepare function returns a non-terminated string.

This is what the code was actually meant to look like:

((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';

I don't see any reason, though, for implementing this function in such a complicated and unconventional way. Because of this overcomplicating, what should have been a small and simple function ended up with a critical bug in it. This code can be reduced to the following:

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    strcpy(cpy_mntpt, mntpt);
  }
  return cpy_mntpt;
}

Example 2

void myMemCpy(void *dest, void *src, size_t n) 
{ 
   char *csrc = (char *)src; 
   char *cdest = (char *)dest; 
   for (int i=0; i<n; i++) 
     cdest[i] = csrc[i]; 
}

We didn't catch this one; I came across it on StackOverflow: C and static Code analysis: Is this safer than memcpy?

Well, if you check this function with PVS-Studio, it will expectedly issue the following warnings:

  • V104 Implicit conversion of 'i' to memsize type in an arithmetic expression: i < n test.cpp 26
  • V108 Incorrect index type: cdest[not a memsize-type]. Use memsize type instead. test.cpp 27
  • V108 Incorrect index type: csrc[not a memsize-type]. Use memsize type instead. test.cpp 27

Indeed, this code has a flaw, and it was pointed out in the replies on StackOverflow. You can't use a variable of type int as an index. In a 64-bit program, an int variable would most certainly (we don't talk about exotic architectures now) be 32 bits long and the function would be able to copy only as much as INT_MAX bytes, i.e. not more than 2 GB.

With a larger buffer to copy, a signed integer overflow will occur, which is undefined behavior in C and C++. By the way, don't try to guess how exactly the bug would manifest itself. Surprisingly, it's quite a complicated topic, which is elaborated on in the article "Undefined behavior is closer than you think".

The funniest thing is that the code shown above was written in an attempt to eliminate some warning of the Checkmarx analyzer triggered by a call of the memcpy function. The wisest thing the programmer could come up with was to reinvent the wheel. But the resulting copy function – however simple – ended up flawed. The programmer probably made the things even worse than they had already been. Rather than try and find the cause of the warning, he or she chose to conceal the issue by writing their own function (thus confusing the analyzer). Besides, they made a mistake of using an int variable as a counter. And yes, code like that may not be optimizable. Using a custom function instead of the existing efficient and optimized function memcpy is not an efficient decision. Don't do that :)

Conclusion

Well, it's only the start of the journey, and it might well take me a few years before I collect enough examples to write a profound article on this topic. Actually, it's only now that I'm starting to keep an eye for such cases. Thanks for reading, and be sure to try PVS-Studio on your C/C++/C#/Java code – you may find something interesting.

Popular related articles
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept