Larger than 0 does not mean 1

The following code fragment is taken from CoreCLR project. The code has an error that analyzer diagnoses in the following way: V698 Expression ‘memcmp(….) == -1’ is incorrect. This function can return not only the value ‘-1’, but any negative value. Consider using ‘memcmp(….) < 0’ instead.

image_6.png

bool operator( )(const GUID& _Key1, const GUID& _Key2) const
  { return memcmp(&_Key1, &_Key2, sizeof(GUID)) == -1; }

Explanation

Let’s have a look at the description of memcmp() function:

int memcmp ( const void * ptr1, const void * ptr2, size_t num );

It compares the first num bytes of the block of memory pointed by ptr1 to the first num bytes pointed by ptr2, returning zero if they all match, or a value different from zero representing which is greater, if they do not.

Return value:

  • < 0 – the first byte that does not match in both memory blocks has a lower value in ptr1 than in ptr2 (if evaluated as unsigned char values).
  • == 0 – the contents of both memory blocks are equal.
  • > 0 – the first byte that does not match in both memory blocks has a greater value in ptr1 than in ptr2 (if evaluated as unsigned char values).

Note that if blocks aren’t the same, then the function returns values greater than or less than zero. This is important! You cannot compare the results of such functions as memcmp(), strcmp(), strncmp(), etc. with the constants 1 and -1.

Interestingly, the wrong code, where the result is compared with the 1/ -1 can work as the programmer expects for many years. But this is sheer luck, nothing more. The behavior of the function can unexpectedly change. For example, you may change the compiler, or the developers will optimize memcmp() in a new way, so your code will cease working.

Correct code

bool operator( )(const GUID& _Key1, const GUID& _Key2) const
  { return memcmp(&_Key1, &_Key2, sizeof(GUID)) < 0; }

Recommendation

Don’t rely on the way the function is working now. If the documentation says that a function can return values less than or greater than 0, it does mean it. It means that the function can return -10, 2, or 1024. The fact that you always see it return -1, 0, or 1 doesn’t prove anything.

By the way, the fact that the function can return such numbers as 1024, indicates, that the result of memcmp() execution cannot be stored in the variable of char type. This is one more wide-spread error, whose consequences can be really serious. Such a mistake was the root of a serious vulnerability in MySQL/MariaDB in versions earlier than 5.1.61, 5.2.11, 5.3.5, 5.5.22. The thing is, that when a user connects to MySQL/MariaDB, the code evaluates a token (SHA from the password and hash) that is then compared with the expected value of memcmp() function. But on some platforms the return value can go beyond the range [-128..127] As a result, in 1 out of 256 cases the procedure of comparing hash with an expected value always returns true, regardless of the hash. Therefore, a simple command on bash gives a hacker root access to the volatile MySQL server, even if the person doesn’t know the password. The reason for this was the following code in the file ‘sql/password.c’:

typedef char my_bool;
...
my_bool check(...) {
  return memcmp(...);
}

A more detailed description of this issue can be found here: Security vulnerability in MySQL/MariaDB.

Written by Andrey Karpov.

This error was found with PVS-Studio static analysis tool.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s