PVS-Studio: analyze the code of the ReactOS operating system

Having checked the ReactOS code, I was able to fulfill three of my desires at once. First, I have long wanted to write an article about an ordinary project. It is not interesting to check the code of such projects as Chromium. It is too high quality and, in order to maintain this quality, resources that are not available in ordinary projects are spent. Secondly, there was a good example where you can show how static analysis is needed in a large project, especially if it is developed by a heterogeneous distributed team. Thirdly, I received confirmation that PVS-Studio is getting better and more useful.

PVS-Studio is getting better and better

I will start from the last moment, regarding the usefulness of the PVS-Studio tool. ReactOS indirectly confirms that PVS-Studio is developing in the right direction. Here's the news about testing ReactOS with a heavyweight like Coverity - “Static Analysis in Coverity” [ 1 ]. I, of course, know and understand that Coverity’s possibilities are far from us. But, nevertheless, where, thanks to Coverity, “several new mistakes were found”, PVS-Studio finds them a whole car and a small cart. At the same time, no code should be sent anywhere. You can just take and check the project. So we are on the right track.

What is ReactOS?

ReactOS is a modern, free and open operating system based on the Windows XP / 2003 architecture. The system was written from scratch and has as its goal the repetition of the Windows-NT architecture created by Microsoft, from the hardware to the application layer. The source code for C, C ++ and assembler is about 220 megabytes.

Various links:

• Official site of the project ReactOS .
• How to participate in the development of ReactOS .
• Wikipedia. ReactOS .
• An article in Computerra. ReactOS: catch up and overtake .
• Lurkmore. ReactOS .

Errors in ReactOS

Now let's talk about the carriage of errors that I encountered in the ReactOS code. Of course, they all will not enter the article. Here I posted a text file with a description of the errors that I noticed during the analysis. The file contains diagnostic messages with file names and line numbers. I also highlighted the errors in the short code and made some comments. Therefore, those who want to make edits in ReactOS are better guided by this file, and not by the article.
')
Better yet, download and test the project yourself using PVS-Studio. After all, I am not familiar with the project and wrote out only those errors that are clear to me. For many code fragments, I don’t know if they contain errors or not. So my analysis is rather superficial. The key to check out select.

Errors that can be found in ReactOS various. Just a zoo. There are typos of one character.

BOOL WINAPI GetMenuItemInfoA(...) { ... mii->cch = mii->cch; ... } 

In fact, it should be written like this: "mii-> cch = miiW-> cch;". Lost the letter 'W'. As a result, programs can no longer trust the function GetMenuItemInfoA.

And here is another typo in one character. This time the comparison of two names does not work correctly.

 static void _Stl_loc_combine_names(_Locale_impl* L, const char* name1, const char* name2, locale::category c) { if ((c & locale::all) == 0 || strcmp(name1, name1) == 0) ... } 

There is confusion between the && and & operator. A very common mistake. I meet in almost every project where they work with bits or file attributes.

 static LRESULT APIENTRY ACEditSubclassProc() { ... if ((This->options && ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) ... } 

The correct code should look like this "(This-> options & ACO_AUTOSUGGEST)". The example below contains a related error, due to which the entire condition is always false.

 void adns__querysend_tcp(adns_query qu, struct timeval now) { ... if (!(errno == EAGAIN || EWOULDBLOCK || errno == EINTR || errno == ENOSPC || errno == ENOBUFS || errno == ENOMEM)) { ... } 

If you look closely, you can see the insidious fragment: "|| EWOULDBLOCK ||".

By the way, in ReactOS there were many conditions that are always true or false. Some are not scary, because, for example, they are located in the assert () macro. But there are, in my opinion, critical ones.

 INT WSAAPI connect(IN SOCKET s, IN CONST struct sockaddr *name, IN INT namelen) { ... /* Check if error code was due to the host not being found */ if ((Status == SOCKET_ERROR) && (ErrorCode == WSAEHOSTUNREACH) && (ErrorCode == WSAENETUNREACH)) { ... } 

Agree that the implementation of such functions as “connect” should be tested as fully as possible. And here we have a condition that is always false. It is not so easy to quickly notice a defect, so I will highlight the essence of the error:

 (ErrorCode == 10065) && (ErrorCode == 10051) 

By the way, the part related to sockets generally looks raw. Perhaps this is due to the fact that in the Linux world, SOCKET is usually declared as a signed type. And in Windows it is unsigned:

 typedef UINT_PTR SOCKET; 

As a result, we have various errors in comparison operations:

 void adns_finish(adns_state ads) { ... if (ads->tcpsocket >= 0) adns_socket_close(ads->tcpsocket); ... } 

The expression "ads-> tcpsocket> = 0" does not make sense, since it is always true.

There are just strange fragments. Most likely, these are unfinished and forgotten sections of code.

 if (ERROR_SUCCESS == hres) { Names[count] = HeapAlloc(GetProcessHeap(), 0, strlenW(szValue) + 1); if (Names[count]) strcmpW(Names[count], szValue); } 

Why call "strcmpW" if the result is not used in any way?

There are errors associated with the priority of operations.

 VOID NTAPI AtapiDmaInit(...) { ... ULONG treg = 0x54 + (dev < 3) ? (dev << 1) : 7; ... } 

I will place brackets to make it clear how this expression actually works:

 ULONG treg = (0x54 + (dev < 3)) ? (dev << 1) : 7; 

The following error is definitely found in any large project. There are a couple in ReactOS. This is an extra semicolon - ';'.

 BOOLEAN CTEScheduleEvent(PCTE_DELAYED_EVENT Event, PVOID Context) { ... if (!Event->Queued); { Event->Queued = TRUE; Event->Context = Context; ExQueueWorkItem(&Event->WorkItem, CriticalWorkQueue); } ... } 

I also like errors with the initialization of array elements. I do not know why. They are touching. Perhaps I recall my first experiments with arrays in the Basic language.

 HPALETTE CardWindow::CreateCardPalette() { ... //include button text colours cols[0] = RGB(0, 0, 0); cols[1] = RGB(255, 255, 255); //include the base background colour cols[1] = crBackgnd; //include the standard button colours... cols[3] = CardButton::GetHighlight(crBackgnd); cols[4] = CardButton::GetShadow(crBackgnd); ... } 

You can continue to bring different interesting parts of the code. Unfortunately, then the article will become too long and you need to stop. Let me remind you that you can look at the other errors that I found in ReactOS in this file . I'll just bring this piece of code for sweets:

 #define SWAP(a,b,c) c = a;\ a = b;\ a = c 

Example of use:

 BOOL FASTCALL IntEngGradientFillTriangle(...) { ... SWAP(v2,v3,t); ... } 

It is a masterpiece.

Static code analysis

I consider ReactOS a very good example of a project where regular static code analysis is essential. And it's not about the qualifications of the developers. The project is large and contains a variety of subsystems. This means that a large number of people always work on such a project. And in a large team, someone always programs worse, someone better. Someone uses one style, someone else. But no one is immune from mistakes. Look here.

One person picked up and wrote to ReactOS like this:

 if ((res = setsockopt(....) == -1)) 

The code does not do what it wants. The correct option: if ((res = setsockopt (....)) == -1). If you follow the practice of writing a constant at the beginning, you will never accidentally make an erroneous assignment inside an if statement. We have a different type of error here. But if you write code according to the above rule, you will not be able to make a mistake in the expression under consideration: "if (-1 == res = setsockopt (....))".

But the knowledge of this practice does not prevent another person from making a mistake in an alternative way.

 static DWORD CALLBACK RegistrationProc(LPVOID Parameter) { ... if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR, UnknownError, sizeof(UnknownError) / sizeof(UnknownError[0] - 20))) ... } 

Here, the constant 0 is beautifully written at first. The only thing is that the parenthesis is not closed where it should be. Common typo.

Why do I have all these examples? And the fact that none of us programmers are perfect. And neither the coding standard, nor the programming technology, nor the self-control guarantee the absence of errors in the code.

In large projects, assistive technologies such as dynamic and static analysis are simply needed. I emphasize:

I believe that static code analysis should be a mandatory element of the development cycle of ReactOS and other large projects.

I will explain my statement. In such systems, it is impossible to approach 100% of the code coverage when testing using unit tests or regression tests. I will clarify a little. Of course you can, but the costs of creating and maintaining such tests become unacceptably high.

The reason is that the number of possible system states and the possible ways of executing the code branches are very large. Some branches rarely get control, but that does not make them unnecessary. It is here that it turns out to see the advantage that static analysis has. It checks the entire code, regardless of how often it gets control in the course of the program.

An example of checking code that rarely receives control:

 static HRESULT STDMETHODCALLTYPE CBindStatusCallback_OnProgress(...) { ... if (This->szMimeType[0] != _T('\0')) _tprintf(_T("Length: %I64u [%s]\n"), This->Size, This->szMimeType); else _tprintf(_T("Length: %ull\n"), This->Size); ... } 

Most likely, the code was originally written incorrectly. Then someone noticed that the message was formed incorrectly and made a correction by writing "% I64u". But he did not pay attention to the code in the neighborhood. And there is still an incorrect format "% ull". Apparently the branch is called extremely rarely. Static analysis will not miss this. Actually, he did not miss it, since I can demonstrate this example.

Another good example is the large number of memory cleanup errors that I saw ReactOS. Why so many of them, I understand. No one is testing whether the memory is full or not. Firstly, it is difficult to realize that in these simple places you can make a mistake. And secondly, it is not so easy to check whether some temporary buffer was cleared inside the function or not. Here, static analysis is again at altitude. I will give only a couple of examples. In fact, I counted at least 13 errors of filling arrays with a constant value.

 #define MEMSET_BZERO(p,l) memset((p), 0, (l)) char *SHA384_End(SHA384_CTX* context, char buffer[]) { ... MEMSET_BZERO(context, sizeof(context)); ... } 

We only clear the first bytes of the array, since sizeof (context) returns the pointer size, not the structure.

 #define RtlFillMemory(Destination, Length, Fill) \ memset(Destination, Fill, Length) #define IOPM_FULL_SIZE 8196 HalpRestoreIopm(VOID) { ... RtlFillMemory(HalpSavedIoMap, 0xFF, IOPM_FULL_SIZE); ... } 

Arguments are confused when using the RtlFillMemory macro. The call should be like this:

 RtlFillMemory(HalpSavedIoMap, IOPM_FULL_SIZE, 0xFF); 

Tabs and spaces again

I want to ask in advance not to start a new heated discussion on this topic in the comments. I'll just give my opinion. You can agree with him or not. But it is not worth discussing.

There are two irreconcilable camps. Some use tabs in the code, as this allows you to adjust the display of the code for themselves [ 2 ]. Others say that it still does not work and that there is no reasonable reason to use tabs. From tabbing only mischief and driving about formatting. I am one of them [ 3 ].

One may say that the tabulation should be used correctly and then everything will be fine. Unfortunately, it is said by those who work on one project and are not confronted with the outside world. In any open or just a large project, it is not possible to achieve normal code design, if you allow tabs to be used in any form.

I will not engage in abstract reasoning. This time I’ll just give my opponents a good example. Now such an example would be ReactOS code.

The ReactOS coding standard [ 4 ] says a good rule from a theoretical point of view:
Generic note about TABs usage: Don't use TABs for formatting; use TABs for indenting only and use only spaces for formatting.

 Example: 
 NTSTATUS
 SomeApi (IN Type Param1,
 [spaces] IN Type Param2)
 {
 [TAB] ULONG MyVar;
 [TAB] MyVar = 0;
 [TAB] if ((MyVar == 3) &&
 [TAB] [sp] (Param1 == TRUE))
 [TAB] {
 [TAB] [TAB] CallSomeFunc ();
 ... 

Tab fans are happy. However, I open the source codes of ReactOS and observe corrupted formatting in many places. Why?





The answer is of course obvious. Because it is difficult to remember where to press TAB, and where to put some spaces, if this is not the only project you are working with. Here people are constantly wrong. And if so, there is nothing to be theorists, but you have to be practitioners. We must take and prohibit the use of tabs in general. And then everything will be the same for everyone, and the culprit who begins to use tabulation is easy to find and make him a suggestion.

This is not a step back in the design of the code! This is a step forward! This is the next level of awareness. The theoretical beauty of customizable indentation is not combined with practice. First of all, it is important to provide a clear view of the code and ease of development in a large team. Google understands this. And in their standard for formatting only spaces are used [ 5 ]. To those who advocate the use of tabulation, I once again recommend thinking about why the distributed team of high-class professionals who develop Chromium chose exactly the gaps.

Once again. The theoretical beauty of customizable indentation is not combined with practice. No matter how beautiful the theory sounds, if it doesn't work. That is exactly what happens in ReactOS.

I recommend the team developing ReactOS to modify their standard and stop using tabulation. Any tabulation should be regarded as an error and be eliminated from the code.

By the way, this practice will make it possible to find such ugliness in ReactOS code:

 BOOLEAN KdInitSystem(IN ULONG BootPhase, IN PLOADER_PARAMETER_BLOCK LoaderBlock) { ... /* Check if this is a comma, a space or a tab */ if ((*DebugOptionEnd == ',') || (*DebugOptionEnd == ' ') || (*DebugOptionEnd == ' ')) ... } 

The last comparison is a comparison with the tab character and not with a space, as it may seem. Normal code should look like this: "(* DebugOptionEnd == '\ t')".

Note for tabs. I don’t need to tell me again how to use tabs correctly. And this is not my code. Look, there is a very specific ReactOS project. It has poorly formatted code. Think what actions will allow the new programmer to open the project code and not guess what size of the tab character he needs to set in the editor settings. Thoughts in the spirit of “it was necessary to immediately write correctly” have no practical value.

Bibliographic list

1. ReactOS N79 news release. Static analysis in Coverity. http://www.viva64.com/go.php?url=722
2. It's time to start using spaces instead of tabs in the code. http://www.viva64.com/go.php?url=725
3. It's time to tie tabs to code. http://www.viva64.com/go.php?url=726
4. ReactOS. Coding Style. http://www.viva64.com/go.php?url=724
5. Google C ++ Style Guide. http://www.viva64.com/go.php?url=679