MAME emulator disease: memset()

While analyzing the source codes of various programs I can't help creating associations that each program has a tendency to certain diseases. In many projects you can easily make out patterns of incorrect code that can be found in different project files. In some programs these are Copy-Paste errors, while in others it's "unsigned_integer < 0"-like checks. Each project has its own disease. The sore of the next project (called MAME) we have checked is the memset() function.

MAME is an emulator application designed to recreate the hardware of arcade game systems in software to preserve gaming history by preventing vintage games from being lost or forgotten [1]. Although almost all the project files have the ".c" extension, MAME is actually a C++ project. The source code's size is rather large - 110 Mbytes.

Checking MAME with PVS-Studio was impossible before because it is built with MinGW on Windows. MinGW is a native software port of the GNU Compiler Collection (GCC) under Microsoft Windows [2]. It means that PVS-Studio has to provide correct support of the special features of the GCC syntax and special keywords.

Support of MinGW has been available in PVS-Studio since version 4.70. It is not full yet, but it is enough to check most projects. MAME was one of the first projects to be analyzed.

Note. While performing the analysis, there will be a lot of similar false reports. The odd code fragments are located in several macros widely used in various project parts. It seems at first that there are only false positives - scattered useful messages just get lost among them. However, you can easily fix it by adding just a few comments to suppress the warnings triggered by the macros. See the "Suppression of false alarms" section in the documentation to find out how to do that.

Now let's study the errors we have detected.

Incompletely cleared arrays

As we have already said, you can find a lot of fragments in the MAME project where the memset function is used incorrectly. A typical mistake is filling only a part of an array. Consider a simple example:

PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_pstars_regs'. pgm.c 4458

Number 16 means the number of items in the "m_pstars_regs" array. But it is the number of bytes being filled in the buffer that should be passed into the memset function. As a result, only a part of the array is filled with zeroes.

This is the correct code:

memset(state->m_pstars_regs, 0, 16 * sizeof(UINT32));

The mistake is trivial. Programmers often think that there are few trivial errors in their programs (see the second myth [3]). It's not so. It is very simple and silly mistakes that make up the largest part of errors found in programs.

Do you think the error shown above is a single one? No. Here you are at least 8 other fragments where instances of the same mistake can be found:

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_kb_regs'. pgm.c 4975

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_kb_regs'. pgm.c 4996

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_kb_regs'. pgm.c 5056

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_oldsplus_ram'. pgm.c 5780

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_oldsplus_regs'. pgm.c 5781

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_sysreg'. rungun.c 399

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_ttl_vram'. rungun.c 400

V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_playfield_code'. malzak.c 392

In the example above, the number of items was defined by an absolute number. It is bad. You'd better calculate the array size instead of using constants. Unfortunately, it doesn't help to avoid the error we're discussing.

PVS-Studio: V512 A call of the 'memcpy' function will lead to the '& rawheader[100]' buffer becoming out of range. chd.c 1870

The 'rawheader' array consists of 108 bytes. We want to copy its contents from byte 100 on. The trouble is we will reach outside the array boundaries. We can copy only 8 bytes, yet 20 bytes are actually copied. Unfortunately, I don't know how to fix this code, since I'm not familiar with the program logic.

When using the memset() function, it often happens that only a part of an array is filled. Correspondingly, when you use the memset() function, there may often be errors causing only a part of an array to be copied. Consider the following sample:

V512 A call of the 'memcpy' function will lead to underflow of the buffer 'state->m_spriteram16_2_buffered'. deco32.c 726

Misprints and Copy-Paste

In any project you can see misprints and errors caused by using the Copy-Paste technology. There are few of them in some projects and quite many in others. In MAME these errors are not numerous, yet they are there. Let's study some of them.

V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: ",(%d,". 9900dasm.c 670

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 549, 579. cdrom.c 549

V501 There are identical sub-expressions 'offset != (0x370 >> 1)' to the left and to the right of the '&&' operator. decoprot.c 118

V501 There are identical sub-expressions 'offset != (0x3c0 >> 1)' to the left and to the right of the '&&' operator. decoprot.c 118

V501 There are identical sub-expressions 'offset != 0x2c / 2' to the left and to the right of the '&&' operator. decoprot.c 240

V501 There are identical sub-expressions 'offset != 0xe' to the left and to the right of the '&&' operator. decoprot.c 447

Undefined behavior

Quite a number of warnings generated by PVS-Studio for this project refer to shift operations. These operations lead to undefined behavior. Of course, when you use particular compilers, your code can work properly for many years. That's why we can call these errors potential. They may reveal themselves when moving to a different platform, compilers or optimization switches. To learn more about it, please see the article: "Wade not in unknown waters. Part three." [4].

Consider a couple of samples causing undefined behavior. The first sample:

PVS-Studio: V610 Undefined behavior. Check the shift operator '<. The right operand ('i' = [0..43]) is greater than or equal to the length in bits of the promoted left operand. firetrk.c 111

The 'colortable_source' array contains 44 items. Therefore, the 'i' loop counter takes values from 0 to 43. Number '1' has the int type - it cannot be shifted more than by 31 bits. If you shift it by more bits, it will cause undefined behavior according to the language standard.

Since there are quite few warnings related to shifts, we won't cite them in the article. You can look through the list of these messages in the text file: mame-shift-ub.txt.

Other errors

Besides the functions memset() and memcpy(), there is memcmp() I've almost forgotten about. This function is from the same gang. Fortunately, I've found only one error related to use of this function in MAME.

PVS-Studio: V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. apridisk.c 128

The sizeof() operator calculates the pointer size instead of the number of bytes in a string. As a result, only the first several bytes are compared. We can fix it by defining the 'apr_magic' variable as an array:

PVS-Studio: V547 Expression is always true. Probably the '&&' operator should be used here. mpu4.c 934

The "X != 1 || X != 0" condition is always true. Most probably, the '&&' operator should be written instead of the '||' operator.

Use of a pointer before a check. I will cite only one example of this. I saw other V595 messages too but didn't note them down. In many cases the code works well, as the pointer never equals zero in those fragments. Here is an example of odd code:

PVS-Studio: V595 The 'gfx' pointer was utilized before it was verified against nullptr. Check lines: 2457, 2483. stvvdp2.c 2457

At times I come across some strange code about which I cannot tell for sure if it has an error or not. Maybe there is a Copy-Paste mistake. And maybe everything is correct and the two code branches are actually intended to be identical. Here is an example:

PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. resnet.c 628

Conclusion

As usual, I will stress that these are probably quite not all the errors that PVS-Studio can find in MAME. The task of this article is to show that PVS-Studio is learning to check crossplatform projects. To know how exactly you can integrate into the make-file, please see the documentation. You may also ask us if you have any troubles analyzing projects built with MinGW.

P.S. Review of analysis results currently implies that you need the Visual Studio environment where you can open the report and study it. Manual analysis of the report is very effortful. Maybe we will make a special tool in future that will allow you to conveniently review the report and perform code navigation without having Visual Studio installed.