Geekly Articles each Day
📜 ⬆️ ⬇️

February 31

February 31

Now I am studying the report of the regular verification of the Chromium project and the libraries used in it, using the PVS-Studio code analyzer. Following the results of the check, I have a cycle of articles about the analysis of certain types of errors and how to avoid them. However, one mistake was so much liked that I decided not to postpone its description, but immediately write a little note to the blog.

Our team has already written 5 articles ( 1 , 2 , 3 , 4 , 5 ), devoted to finding errors in the open project Chromium . And, apparently, soon I will write a few more notes to our blog on this topic.

Now I study the report issued by the PVS-Studio analyzer and just write out the errors found. Writing notes is the next step. I like to look at the report at the beginning, and then decide how and what to write about. However, I especially liked one mistake, and I decided to describe it immediately, not postponing it for later.

The bug lives in the Protocol Buffers (protobuf) library used in the Chromium project. Protocol Buffers is a protocol for the serialization (transmission) of structured data, proposed by Google as an effective binary alternative to XML text format.
')
If I saw this error a couple of months ago, I would have passed by it. Well, error and error. However, when I saw it, I immediately remembered the epic failure of cash registers, which recently occurred in Russia. On December 20, the largest retailers and gas station networks across Russia faced disruptions in the operation of new cash registers. The residents of Vladivostok were the first to find out about the problem, then it spread throughout the country as the working day began, affecting Novosibirsk, Barnaul, Krasnoyarsk, Kemerovo and other major cities.

Error in cash registers and an error in the Protocol Buffers are two different errors that are not related to each other. But I wanted to show how such errors can occur. Indeed, defects are often not tailored to cunning algorithms, but are the result of trivial typos. I do not know what was confused in the cash register code, but I know how a stupid typo leads to the inoperability of the ValidateDateTime function designed to check the date in Protocol Buffers. Let's look at the code for this function and look at it.

static const int kDaysInMonth[13] = { 0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; bool ValidateDateTime(const DateTime& time) { if (time.year < 1 || time.year > 9999 || time.month < 1 || time.month > 12 || time.day < 1 || time.day > 31 || time.hour < 0 || time.hour > 23 || time.minute < 0 || time.minute > 59 || time.second < 0 || time.second > 59) { return false; } if (time.month == 2 && IsLeapYear(time.year)) { return time.month <= kDaysInMonth[time.month] + 1; } else { return time.month <= kDaysInMonth[time.month]; } }

The ValidateDateTime function accepts a date as input and must determine whether it is valid or not. At the beginning, basic checks are performed. It is checked that the month number is in the range [1..12], the days are in the range [1..31], the minutes are in the range [0..59] and so on. In the code, all this is clearly visible, and does not require special explanations.

Next comes a more difficult test whether there is a certain day in a particular month. For example, December consists of 31 days, and in November the 31st is not, since there are only 30 days in a month.

To check the days and not to write a set of if statements or a long switch , the auxiliary array kDaysInMonth is used . The array contains the number of days in different months. By the month number, the maximum number of days is extracted from the array and used for verification.

Additionally, it is taken into account that the year may be a leap year, and then in February 1 day more.

In general, the function is written simply and beautifully. That's just wrong.

Due to a typo, days are checked incorrectly. Please note that with the maximum possible number of days in a month, not a day is compared, but a month.

Take another look at the code:

 if (time.month == 2 && IsLeapYear(time.year)) { return time.month <= kDaysInMonth[time.month] + 1; } else { return time.month <= kDaysInMonth[time.month]; }

In the " time.month <= " comparisons, you should not use the member of the month structure, but the day member. Those. The correct code should look like this:

 if (time.month == 2 && IsLeapYear(time.year)) { return time.day <= kDaysInMonth[time.month] + 1; } else { return time.day <= kDaysInMonth[time.month]; }

Naturally, the month number (from 1 to 12) is always less than the number of days in any of the months.

Because of this error, such days as, for example, February 31 or November 31 will be considered as correct.

Here is such an interesting mistake met. Because of it, incorrect dates can be processed, which can theoretically be used later to attack the system. Here, perhaps, I exaggerate, but in fact, vulnerabilities usually look that way. Just somewhere some input data is not verified, and someone guessed that.

This error (or rather, two errors) is detected by means of the following warnings of PVS-Studio:


As you can see, now PVS-Studio also immediately classifies warnings according to Common Weakness Enumeration ( CWE ).

I also want to note that the PVS-Studio analyzer is learning more and more deeply to analyze the code. By itself, the V547 diagnosis is old (it was implemented in 2010). However, let's say, a year ago, the analyzer could not find this error. Now the analyzer is able to look inside the array and understand that the values ​​in the range [28..31] are extracted. At the same time, he understands that 0 in the array is not necessary to take into account, since the possible range of time.month is [1..12]. If the month number was, say, 100, then there was an exit from the function and the analyzer understands this.

As a result, the analyzer sees that the following range comparisons occur:


Therefore, the conditions are always true, what the analyzer and the warning. Here is such a deep analysis. As you see, we not only add new warnings in PVS-Studio, but also refine the Data-Flow analysis, which affects the quality of the existing diagnostics.

Why is the first range [2, 2] represented by only one number 2? The fact is that the clarifying condition time.month == 2 is taken into account.

One should ask: how can one improve the style in order to protect oneself from such mistakes?

I do not know. The function is simple and written in good style. It’s just human nature to make mistakes, and he can make such typos. No professional is immune from such errors.

We can only recommend to write more carefully unit tests and use professional static code analyzers, such as PVS-Studio.

Thanks for attention. I will go further to study the report.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. February 31 .

Source: https://habr.com/ru/post/346128/

More articles:


All Articles