FCEUX

BUG OF THE MONTH | Buffer overflow

There is an open-source project test base that we use for our core’s regression testing. The project test base contains the FCEUX emulator. The upgraded analyzer found an interesting error in the Assemble function.

int Assemble(unsigned char *output, int addr, char *str) {
  output[0] = output[1] = output[2] = 0;
  char astr[128],ins[4];
  if ((!strlen(str)) || (strlen(str) > 0x127)) return 1;
  strcpy(astr,str);
  ....
}

Can you see it? To be honest, we did not notice it immediately and our first thought was, “Oh no, we broke something!” Then we saw what was up and took a minute to appreciate the advantages of static analysis.

PVS-Studio warned: V512 A call of the ‘strcpy’ function will lead to overflow of the buffer ‘astr’. asm.cpp 21

Still don’t see the error? Let’s go through the code step by step. To start with, we’ll remove everything irrelevant:

int Assemble(char *str) {
  char astr[128];
  if ((!strlen(str)) || (strlen(str) > 0x127)) return 1;
  strcpy(astr,str);
  ....
}

The code above declares a 128-byte array. The plan is to verify a string and then pass it to the strcpy function that copies the string to the array. The string should not be copied if it is empty or contains over 127 characters (not counting the terminal zero).

So far, all is well and good, right? Wait, wait, wait. What do we see here? What kind of a constant is 0x127?!

It’s not 127 at all. Far from it!

This constant is set in hexadecimal notation. If you convert it to decimal, you get 295.

So, the code above is equivalent to the following:

int Assemble(char *str) {
  char astr[128];
  if ((!strlen(str)) || (strlen(str) > 295)) return 1;
  strcpy(astr,str);
  ....
}

As you can see, the str string check does not prevent possible buffer overflows. The analyzer correctly warns you about the problem.

Previously, the analyzer could not find the error. The analyzer could not understand that both strlen function calls work with the same string. And the string does not change between them. Although things like this one are obvious to developers, this is not the case for the analyzer. It needs to be taught expressly.

Now PVS-Studio warns that the str string length is in the [1..295] range, and thus may exceed the array bounds if copied to the astr buffer.

Please click here to see more bugs from this project.

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 )

Google photo

You are commenting using your Google 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 )

Connecting to %s

This site uses Akismet to reduce spam. Learn how your comment data is processed.