Development of large complex projects is impossible without the use of programming techniques and tools helping to monitor the quality of the code. First, it requires a literate coding standard, code reviews, unit tests, static and dynamic code analyzers. All this helps to detect defects in code at the earliest stages of development. This article demonstrates the abilities of a PVS-Studio static analyzer in detecting bugs and security weaknesses in code of the Android operating system. We hope that the article will attract readers' attention to the methodology of static code analysis, and they will be willing to embed it in the process of developing their own projects.
It has been a year since writing the big article about errors in Tizen operating system and once again, we felt like making a no less exciting research of an operating system. The choice fell on Android.
The code for the Android operating system is well tested and qualitative. At least Coverity static analyzer is used when developing it, as evidenced by the following comments:
/* Coverity: [FALSE-POSITIVE error] intended fall through */
/* Missing break statement between cases in switch statement */
/* fall through */Generally speaking, this is an interesting, high-quality project, and finding errors in it is a challenge for our PVS-Studio analyzer.
I think, the length of this article itself demonstrates to the reader that the PVS-Studio analyzer perfectly coped with the task and found a lot of defects in the code of the operating system.
In this article you will find links to the Common Weakness Enumeration (CWE). I'd like to explain the reason of referring to this list and why it's important from the security perspective.
Very often the cause of vulnerabilities in applications is not a tricky set of circumstances, but a simple programming error. Here it would be appropriate to cite this quote from the website prqa.com:
'The National Institute of Standards and Technology (NIST) reports that 64% of software vulnerabilities stem from programming errors and not a lack of security features'.
You can see some examples of simple errors that resulted in vulnerabilities in projects such as MySQL, iOS, NAS, illumos-gate in the article "How Can PVS-Studio Help in the Detection of Vulnerabilities?".
Accordingly, many vulnerabilities can be avoided by timely detection and correction of common errors. Here Common Weakness Enumeration enters the stage.
Errors are different, and not all errors are dangerous from the security point of view. The errors that might potentially cause a vulnerability, are collected in Common Weakness Enumeration. This list is updated, and certainly, there are errors that can lead to security vulnerabilities, but they haven't gotten on this list.
However, if the error is classified according to CWE, it means that it is theoretically possible that it can be used as a vulnerability (CVE). Yes, it is not very likely. CWE turns into CVE very seldom. However, if you want to protect your code from security vulnerabilities, you should find as many bugs as described in CWE, and eliminate them.
Schematic relationship between PVS-Studio, errors, CWE and CVE is shown on the Figure:
Some errors are classified as CWE. Many of these bugs can be detected by PVS-Studio, so that these defects will not become vulnerabilities (CVE).
We can say for sure that PVS-Studio reveals many potential vulnerabilities before they do any harm. So, PVS-Studio is a static application security testing tool (SAST).
Now, I think it is clear why, describing errors, I found it important to point out the way they are classified according to CWE. With this clarification is becomes easier to show the importance of static analysis application in important projects, which definitely include operating systems.
To do the analysis we used the PVS-Studio analyzer, version 6.24. The analyzer currently supports the following languages and compilers:
Note. Perhaps, some of our readers have missed the news that we supported working on macOS environment, and this publication will be interesting for them: "PVS-Studio is now available on macOS: 64 weaknesses in the Apple's XNU Kernel".
The process of checking Android source code did not include any problems, so I will not dwell on it for long. Rather, the problem was my being busy with other tasks, due to which I have not found time and energy to review the report so carefully, as I wanted. However, even a cursory view proved to be more than enough to gather a large collection of interesting errors for a huge article.
The most important thing: I'd like to ask Android developers not only to fix bugs described in the article, but also to undertake a more thorough independent analysis. I looked though the analyzer report superficially and could miss many serious errors.
When performing the first check, the analyzer generates many false positives, but this is not a problem. Our team is ready to help with recommendations on how to configure the analyzer to reduce the number of false positives. We are also ready to provide a license key for a month or more if you need it. So, write to us, we will help and give some advice.
Now let's see what kind of errors and potential vulnerabilities I managed to find. I hope you will like what the PVS-Studio static code analyzer can detect. Enjoy the reading!
The analyzer finds expressions abnormal if they are always true or false. Such warnings, according to the Common Weakness Enumeration, are classified as:
The analyzer triggers a lot of such warnings, and, unfortunately, most of them are false positives for the Android code. In doing so, the analyzer is not to blame. The code is just written in such a way. I'll demonstrate it using a simple example.
#if GENERIC_TARGET
const char alternative_config_path[] = "/data/nfc/";
#else
const char alternative_config_path[] = "";
#endif
CNxpNfcConfig& CNxpNfcConfig::GetInstance() {
....
if (alternative_config_path[0] != '\0') {
....
}Here the analyzer issues a warning: V547 CWE-570 Expression 'alternative_config_path[0] != '\0'' is always false. phNxpConfig.cpp 401
The issue is that the GENERIC_TARGET macro is not defined, and from the analyzer perspective it looks as follows:
const char alternative_config_path[] = "";
....
if (alternative_config_path[0] != '\0') {The analyzer just has to issue a warning, because the string is empty, and there is always a terminal null by the zero offset. Thus, the analyzer is formally right, issuing a warning. However, from a practical point of view, this warning is not very helpful.
Unfortunately, nothing can be done with such situations. So, one has to consistently look through such warnings and mark many places as false positives so that the analyzer will not issue warnings for these lines of code. It really should be done, because, in addition to pointless warnings a big number of real defects will be found.
I must admit honestly that I was not interested in careful viewing of warnings of this type, and I looked through them superficially. But even this was enough to show that such diagnostics are rather useful and find interesting bugs.
I would like to start with a classic situation when a function which compares two objects is implemented incorrectly. Why classic? This is a typical pattern of errors, which we constantly come across in various projects. Most likely, there are three reasons for its occurrence:
A more detailed description of these ideas is given in the article "The Evil within the Comparison Functions".
static inline bool isAudioPlaybackRateEqual(
const AudioPlaybackRate &pr1,
const AudioPlaybackRate &pr2)
{
return fabs(pr1.mSpeed - pr2.mSpeed) <
AUDIO_TIMESTRETCH_SPEED_MIN_DELTA &&
fabs(pr1.mPitch - pr2.mPitch) <
AUDIO_TIMESTRETCH_PITCH_MIN_DELTA &&
pr2.mStretchMode == pr2.mStretchMode &&
pr2.mFallbackMode == pr2.mFallbackMode;
}So, here is our classic function comparing two objects of the AudioPlaybackRate type. I suppose, the reader suspects that it is wrong. The PVS-Studio analyzer notices here even two typos:
The fields pr2.mStretchMode and pr2.mFallbackMode are compared with each other. It turns out that the function compares the objects not accurately enough.
The following pointless comparison lives in the comparison function, which stores information about a fingerprint to a file.
static void saveFingerprint(worker_thread_t* listener, int idx) {
....
int ns = fwrite(&listener->secureid[idx],
sizeof(uint64_t), 1, fp);
....
int nf = fwrite(&listener->fingerid[idx],
sizeof(uint64_t), 1, fp);
if (ns != 1 || ns !=1) // <=
ALOGW("Corrupt emulator fingerprints storage; "
"could not save fingerprints");
fclose(fp);
return;
}The incorrectness of this code is revealed by two diagnostics:
There is no situation processing when the second call of the fwrite function cannot record the data in a file. In other words, the value of the nf variable isn't checked. The correct check should look as follows:
if (ns != 1 || nf != 1)Let's move on to the next error related to using the operator &.
#define O_RDONLY 00000000
#define O_WRONLY 00000001
#define O_RDWR 00000002
static ssize_t verity_read(fec_handle *f, ....)
{
....
/* if we are in read-only mode and expect to read a zero
block, skip reading and just return zeros */
if (f->mode & O_RDONLY && expect_zeros) {
memset(data, 0, FEC_BLOCKSIZE);
goto valid;
}
....
}PVS-Studio warning: V560 CWE-570 A part of conditional expression is always false: f->mode & 00000000. fec_read.cpp 322
Note that the O_RDONLY constant is zero. This makes the expression f->mode & O_RDONLY pointless because it is always 0. It turns out that the condition of the operator if is never executed, and statement-true is dead code.
The correct check should look as follows:
if (f->mode == O_RDONLY && expect_zeros) {Now let's take a look at a classic typo where a developer forgot to write a part of the condition.
enum {
....
CHANGE_DISPLAY_INFO = 1 << 2,
....
};
void RotaryEncoderInputMapper::configure(nsecs_t when,
const InputReaderConfiguration* config, uint32_t changes) {
....
if (!changes ||
(InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {
....
}PVS-Studio warning: V768 CWE-571 The enumeration constant 'CHANGE_DISPLAY_INFO' is used as a variable of a Boolean-type. InputReader.cpp 3016
The condition is always true, because the operand InputReaderConfiguration::CHANGE_DISPLAY_INFO is a constant, equal to 4.
If you look at the way the nearby code is written, it becomes clear that the condition, in fact, must be as follows:
if (!changes ||
(changes & InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {The following meaningless comparison was found in the loop operator.
void parse_printerAttributes(....) {
....
ipp_t *collection = ippGetCollection(attrptr, i);
for (j = 0, attrptr = ippFirstAttribute(collection);
(j < 4) && (attrptr != NULL);
attrptr = ippNextAttribute(collection))
{
if (strcmp("....", ippGetName(attrptr)) == 0) {
....TopMargin = ippGetInteger(attrptr, 0);
} else if (strcmp("....", ippGetName(attrptr)) == 0) {
....BottomMargin = ippGetInteger(attrptr, 0);
} else if (strcmp("....", ippGetName(attrptr)) == 0) {
....LeftMargin = ippGetInteger(attrptr, 0);
} else if (strcmp("....", ippGetName(attrptr)) == 0) {
....RightMargin = ippGetInteger(attrptr, 0);
}
}
....
}PVS-Studio warning: V560 CWE-571 A part of conditional expression is always true: (j < 4). ipphelper.c 926
Note that the value of the variable j is incremented nowhere. This means that the subexpression (j < 4) is always true.
The greatest number of useful triggerings of the PVS-Studio analyzer, related to always true/false conditions, refers to the code, which checks the result of object creation using the new operator. In other words, the analyzer detects the following pattern of code:
T *p = new T;
if (p == nullptr)
return ERROR;Such checks are meaningless. If new fails to allocate memory for an object, the std::bad_alloc exception will be generated and the case will not even reach the point where the pointer value is checked.
Note. The operator new can return nullptr, if you write new (std::nothrow). However, this does not relate to errors in question. The PVS-Studio analyzer takes (std::nothrow) into account and does not issue a warning if the object is created in this way.
It might seem that such errors are harmless. Well, it's not a big deal, just an extra check, which never works. Anyway, an exception will be thrown and handled somewhere. Unfortunately, some developers place actions releasing resources, etc. in statement-true of the operator if. Since this code is not executed, it can lead to memory leaks and other bugs.
Let's consider one of these cases I have noticed in the Android code.
int parse_apk(const char *path, const char *target_package_name)
{
....
FileMap *dataMap = zip->createEntryFileMap(entry);
if (dataMap == NULL) {
ALOGW("%s: failed to create FileMap\n", __FUNCTION__);
return -1;
}
char *buf = new char[uncompLen];
if (NULL == buf) {
ALOGW("%s: failed to allocate %" PRIu32 " byte\n",
__FUNCTION__, uncompLen);
delete dataMap;
return -1;
}
....
}PVS-Studio warning: V668 CWE-570 There is no sense in testing the 'buf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. scan.cpp 213
Please note that if the allocation of the second memory block fails, the programmer attempts to release the first block:
delete dataMap;Now this code never gets control. It is dead code. If an exception appears, a memory leak will occur.
It is fundamentally wrong to write such code. Smart pointers come up for such cases.
In general, the PVS-Studio analyzer has detected 176 places in Android where a check of a pointer is performed after creating objects using new. I didn't start estimating the severity of each piece of code. Sure, I'll not clutter the article with all these warnings. All wishing can see other warnings in the file Android_V668.txt.
Dereferencing of a null pointer causes undefined behavior of a program, so it's quite useful to find and fix such places. Depending on the situation, the PVS-Studio analyzer can classify these errors according to the Common Weakness Enumeration as follows:
I often find such errors in code responsible for handling non-standard or incorrect situations. No one tests such code and an error can live a long life in it. Now we'll consider this very case.
bool parseEffect(....) {
....
if (xmlProxyLib == nullptr) {
ALOGE("effectProxy must contain a <%s>: %s",
tag, dump(*xmlProxyLib));
return false;
}
....
}PVS-Studio warning: V522 CWE-476 Dereferencing of the null pointer 'xmlProxyLib' might take place. EffectsConfig.cpp 205
If the xmlProxyLib pointer is equal to nullptr, a programmer issues a debugging message which requires a dereference of this very pointer. Oops ...
Now let's see a more interesting error.
static void soinfo_unload_impl(soinfo* root) {
....
soinfo* needed = find_library(si->get_primary_namespace(),
library_name, RTLD_NOLOAD, nullptr, nullptr);
if (needed != nullptr) { // <=
PRINT("warning: couldn't find %s needed by %s on unload.",
library_name, si->get_realpath());
return;
} else if (local_unload_list.contains(needed)) {
return;
} else if (needed->is_linked() && // <=
needed->get_local_group_root() != root) {
external_unload_list.push_back(needed);
} else {
unload_list.push_front(needed);
}
....
}PVS-Studio warning: V522 CWE-476 Dereferencing of the null pointer 'needed' might take place. linker.cpp 1847
If the pointer needed != nullptr, a warning is printed, which is very suspicious behavior of the program. Finally it becomes clear that the code contains an error, if you look below and see that if needed == nullptr, a null pointer dereference will occur in the expression needed->is_linked().
Most likely, the operators != and == are simply mixed up. If we make a replacement, the code of the function becomes meaningful and the error disappears.
The largest number of warning on a potential dereference of a null pointer refers to a situation like this:
T *p = (T *) malloc (N);
*p = x;Such functions as malloc, strdup and so on can return NULL, if memory cannot be allocated. Therefore, you cannot dereference pointers returned from these functions without a preliminary check of a pointer.
There are many similar errors, so I'll cite only two simple code fragments: the first with malloc and the second with strdup.
DownmixerBufferProvider::DownmixerBufferProvider(....)
{
....
effect_param_t * const param = (effect_param_t *)
malloc(downmixParamSize);
param->psize = sizeof(downmix_params_t);
....
}PVS-Studio warning: V522 CWE-690 There might be dereferencing of a potential null pointer 'param'. Check lines: 245, 244. BufferProviders.cpp 245
static char* descriptorClassToDot(const char* str)
{
....
newStr = strdup(lastSlash);
newStr[strlen(lastSlash)-1] = '\0';
....
}PVS-Studio warning: V522 CWE-690 There might be dereferencing of a potential null pointer 'newStr'. Check lines: 203, 202. DexDump.cpp 203
Someone may say that these are insignificant errors. If there is not enough memory, the program will simply abort when dereferencing a null pointer, and that's normal. Once there is no memory, it's not worth trying to somehow handle this situation.
Such a person is wrong. Pointers must be checked! I investigated this topic in detail in the article "Why it is important to check what the malloc function returned". I highly recommend all who hasn't read it yet to get acquainted with it.
In short, the danger is that memory writing does not necessarily occur close to a null address. You can write data somewhere very far in the memory page which is not write-protected, and thus cause a slippery error or generally this error can be even used as vulnerability. Let's see what I mean on the example of the function check_size.
int check_size(radio_metadata_buffer_t **metadata_ptr,
const uint32_t size_int)
{
....
metadata = realloc(metadata,
new_size_int * sizeof(uint32_t));
memmove(
(uint32_t *)metadata + new_size_int - (metadata->count + 1),
(uint32_t *)metadata + metadata->size_int -
(metadata->count + 1),
(metadata->count + 1) * sizeof(uint32_t));
....
}PVS-Studio warning: V769 CWE-119 The '(uint32_t *) metadata' pointer in the '(uint32_t *) metadata + new_size_int' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 91, 89. radio_metadata.c 91
I was not sorting out in the logic of the function, but this was not even necessary. The main thing is that a new buffer is created and data is copied in it. If the function realloc returns NULL, then the data will be copied by the address ((uint32_t *)NULL + metadata->size_int - (metadata->count + 1)).
If the value metadata->size_int is great, the consequences will be unfortunate. It turns out that the data is written to a random part of the memory.
By the way, there is another kind of a null pointer dereference that the PVS-Studio analyzer classifies not as CWE-690, but as CWE-628 (invalid argument).
static void
parse_tcp_ports(const char *portstring, uint16_t *ports)
{
char *buffer;
char *cp;
buffer = strdup(portstring);
if ((cp = strchr(buffer, ':')) == NULL)
....
}PVS-Studio warning: V575 CWE-628 The potential null pointer is passed into 'strchr' function. Inspect the first argument. Check lines: 47, 46. libxt_tcp.c 47
The fact of the matter is that the pointer dereferencing will occur when calling the function strchr. So the analyzer interprets this case as a transfer of an incorrect value to a function.
The remaining 194 warnings of this type are cited in a list in the file Android_V522_V575.txt.
By the way, previously considered warnings about a pointer check after calling new give a special piquancy to all these errors. It turns out that there are 195 calls of functions malloc/realloc/strdup and so on, when the pointer is not checked. But there are 176 places, where a pointer is checked after calling new. You must admit, that's a strange approach!
Finally, we need to consider the warnings V595 and V1004 which also involve the use of null pointers.
V595 identifies situations when a pointer is dereferenced and then checked. Let's take a look at a synthetic example:
p->foo();
if (!p) Error();V1004 detects reverse situations, when the pointer was first checked and then a programmer forgot to do it. A synthetic sample:
if (p) p->foo();
p->doo();Let's look at a few fragments of Android code, where there were errors of this type. There is no need to specifically comment on them.
PV_STATUS RC_UpdateBuffer(VideoEncData *video,
Int currLayer, Int num_skip)
{
rateControl *rc = video->rc[currLayer];
MultiPass *pMP = video->pMP[currLayer];
if (video == NULL || rc == NULL || pMP == NULL)
return PV_FAIL;
....
}PVS-Studio warning: V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 385, 388. rate_control.cpp 385
static void resampler_reset(struct resampler_itfe *resampler)
{
struct resampler *rsmp = (struct resampler *)resampler;
rsmp->frames_in = 0;
rsmp->frames_rq = 0;
if (rsmp != NULL && rsmp->speex_resampler != NULL) {
speex_resampler_reset_mem(rsmp->speex_resampler);
}
}PVS-Studio warning: V595 CWE-476 The 'rsmp' pointer was utilized before it was verified against nullptr. Check lines: 54, 57. resampler.c 54
void bta_gattc_disc_cmpl(tBTA_GATTC_CLCB* p_clcb,
UNUSED_ATTR tBTA_GATTC_DATA* p_data) {
....
if (p_clcb->status != GATT_SUCCESS) {
if (p_clcb->p_srcb) {
std::vector<tBTA_GATTC_SERVICE>().swap(
p_clcb->p_srcb->srvc_cache);
}
bta_gattc_cache_reset(p_clcb->p_srcb->server_bda);
} ....
}PVS-Studio warning: V1004 CWE-476 The 'p_clcb->p_srcb' pointer was used unsafely after it was verified against nullptr. Check lines: 695, 701. bta_gattc_act.cc 701
It wasn't very exciting to consider warnings of this type. Among them there are both errors and false positives that occur because of bad or difficult code.
I cited a dozen of useful warnings:
And then I got bored and filtered out warnings of this type. So I don't even know how many of these errors were detected by the analyzer. These warnings are waiting for their hero, who will review them carefully and will make changes in the code.
I'd like to draw the attention of new readers to the errors of this type:
NJ_EXTERN NJ_INT16 njx_search_word(NJ_CLASS *iwnn, ....) {
....
NJ_PREVIOUS_SELECTION_INFO *prev_info =
&(iwnn->previous_selection);
if (iwnn == NULL) {
return NJ_SET_ERR_VAL(NJ_FUNC_NJ_SEARCH_WORD,
NJ_ERR_PARAM_ENV_NULL);
}
....
}PVS-Studio warning: V595 CWE-476 The 'iwnn' pointer was utilized before it was verified against nullptr. Check lines: 686, 689. ndapi.c 686
Some say that there is no error here, because there is "no real pointer dereference". The address of a nonexistent variable is simply calculated. Further, if the pointer iwnn is null, then the function will simply return. Consequently, nothing bad happened because previously we've incorrectly calculated the address of a class member.
No, you cannot reason like this. This code results in undefined behavior, so you can't write like this. Undefined behavior can reveal itself, for example, as follows:
More details on this subject can be found in my article "Null Pointer Dereferencing Causes Undefined Behavior".
Let's consider a serious type of potential vulnerability which is classified according to the Common Weakness Enumeration as CWE-14: Compiler Removal of Code to Clear Buffers.
In short, here is the point: the compiler may remove the memset function call if the buffer is no longer used.
When I write about this kind of vulnerability, there are always comments that this is just a bug in the compiler that needs to be fixed. No, it's not like that. Before you object, please, read the following materials:
Generally speaking, it's all serious. Are there such bugs in Android? Of course, there are. There are lots of them in other projects too: proof :).
Let's get back to the Android code and consider the beginning and the end of a function FwdLockGlue_InitializeRoundKeys, we're not interested in its middle part.
static void FwdLockGlue_InitializeRoundKeys() {
unsigned char keyEncryptionKey[KEY_SIZE];
....
memset(keyEncryptionKey, 0, KEY_SIZE); // Zero out key data.
}PVS-Studio warning: V597 CWE-14 The compiler could delete the 'memset' function call, which is used to flush 'keyEncryptionKey' buffer. The memset_s() function should be used to erase the private data. FwdLockGlue.c 102
The array keyEncryptionKey is created on the stack and stores private information. At the end of the function, a programmer wants to fill this array with zeros so that its data doesn't accidentally end up where it should not. How the information can get to places, where it shouldn't be - is written in the article "Overwriting memory - why?".
To fill an array containing private information with zeros memset is used. The comment "Zero out key data" confirms that we understand everything correctly.
The problem is that there is a very high probability that the compiler will remove the call of memset function when building the release version. Once the buffer is not used after calling memset, the call of memset function itself is redundant from the compiler's point of view.
I cited 10 more warnings in the file Android_V597.txt.
I found one more bug, where the memory isn't cleared, although in this case the memset function has nothing to do with it.
void SHA1Transform(uint32_t state[5], const uint8_t buffer[64])
{
uint32_t a, b, c, d, e;
....
/* Wipe variables */
a = b = c = d = e = 0;
}PVS-Studio warning: V1001 CWE-563 The 'a' variable is assigned but is not used until the end of the function. sha1.c 213
PVS-Studio has revealed an anomaly, that after assigning values to variables, they are no longer used. The analyzer classified this defect as CWE-563: Assignment to Variable without Use. Technically, it's right, though, in fact, here we are dealing with CWE-14. The compiler will remove these assignments, so from the point of view of C and C++ languages they are superfluous. As a result, the previous values of variables a, b, c, d and e, storing private data will remain on the stack.
As long as you are not tired, let's look at a complex case which will require a detailed description on my part.
typedef int32_t GGLfixed;
GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
{
if ((d>>24) && ((d>>24)+1)) {
n >>= 8;
d >>= 8;
}
return gglMulx(n, gglRecip(d));
}PVS-Studio warning: V793 It is odd that the result of the '(d >> 24) + 1' statement is a part of the condition. Perhaps, this statement should have been compared with something else. fixed.cpp 75
A programmer wanted to check that 8 high-order bits of the variable d contain ones but not all bits at once. In other words, the programmer wanted to check that the highest byte contains any value that is different from 0x00 and 0xFF.
He approached this task too creatively. He began with checking that the high-order bits are non-null, having written an expression (d>>24). There are some issues to this expression, but it's more interesting to review the right side of the expression: ((d>>24)+1). The programmer shifts high-order eight bits in a lowest byte. By doing so, he supposes that the highest sign bit is duplicated in all other bits. I.e. if the variable d is equal to 0b11111111'00000000'00000000'00000000, then after the shift the value will be 0b11111111'11111111'11111111'11111111. Having added 1 to the value 0xFFFFFFFF of the int type, the programmer intends to get 0. I.e.: -1+1=0. Thus, by the expression ((d>>24)+1), he checks that not all high-order eight bits are equal to 1. I understand that it is quite complicated, so I ask you to take it slow and try to understand how it all works :).
Now let's go through the point, what is wrong with this code.
When shifting, the highest sign bit is not necessarily "smeared". Here is what is written about this in the standard: "The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined".
The last sentence is the most important for us. So, we met implementation-defined behavior. The way this code will work depends on microprocessor architecture and compiler implementation. After shifting, the highest bits may contain zeros, and if so, the expression ((d>>24)+1) will always be different from 0, i.e. it will always be a true value.
Here is the conclusion: no need to subtilize. The code will be more secure and more understandable, if you write, for example, as follows:
GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
{
uint32_t hibits = static_cast<uint32_t>(d) >> 24;
if (hibits != 0x00 && hibits != 0xFF) {
n >>= 8;
d >>= 8;
}
return gglMulx(n, gglRecip(d));
}Perhaps, I suggested not an ideal variant of code but this code does not have the implementation-defined behavior, and for a reader it will be easier to understand what is checked.
You deserved a cup of tea or coffee. Take a break and we will continue: interesting case of an unspecified behavior is waiting for us.
One of the first questions that I ask an applicant during a job interview is the following: What will the function printf print and why?
int i = 5;
printf("%d,%d", i++, i++)The correct answer is: it's unspecified behavior. The order of evaluation of the actual arguments when calling the function is not defined. Occasionally, I even demonstrate that this code built using Visual C++, displays "6,5" which makes newbies with weak knowledge and spirit become puzzled :).
It might seem that this is a made-up problem. But no, this code can be found in serious software such as Android.
bool ComposerClient::CommandReader::parseSetLayerCursorPosition(
uint16_t length)
{
if (length != CommandWriterBase::kSetLayerCursorPositionLength) {
return false;
}
auto err =
mHal.setLayerCursorPosition(mDisplay, mLayer,
readSigned(), readSigned());
if (err != Error::NONE) {
mWriter.setError(getCommandLoc(), err);
}
return true;
}PVS-Studio warning: V681 CWE-758 The language standard does not define an order in which the 'readSigned' functions will be called during evaluation of arguments. ComposerClient.cpp 836
We're interested in the following line of code:
mHal.setLayerCursorPosition(...., readSigned(), readSigned());By calling readSigned two values are read. But what's the sequence of reading the values is the thing that is impossible to predict. This is a classic case of Unspecified Behavior.
This entire article promotes static code analysis in general and our tool PVS-Studio in particular. However, some errors are simply ideal for demonstration of static analysis abilities. They cannot be easily detected with code reviews, only a tireless program notices them that easily. Let's look at a couple of such cases.
const std::map<std::string, int32_t> kBootReasonMap = {
....
{"watchdog_sdi_apps_reset", 106},
{"smpl", 107},
{"oem_modem_failed_to_powerup", 108},
{"reboot_normal", 109},
{"oem_lpass_cfg", 110}, // <=
{"oem_xpu_ns_error", 111}, // <=
{"power_key_press", 112},
{"hardware_reset", 113},
{"reboot_by_powerkey", 114},
{"reboot_verity", 115},
{"oem_rpm_undef_error", 116},
{"oem_crash_on_the_lk", 117},
{"oem_rpm_reset", 118},
{"oem_lpass_cfg", 119}, // <=
{"oem_xpu_ns_error", 120}, // <=
{"factory_cable", 121},
{"oem_ar6320_failed_to_powerup", 122},
{"watchdog_rpm_bite", 123},
{"power_on_cable", 124},
{"reboot_unknown", 125},
....
};PVS-Studio warnings:
Different values with the same keys are inserted in a sorted associative container std::map. From the point of view of the Common Weakness Enumeration, it is CWE-462: Duplicate Key in Associative List.
The program text is shortened and errors are marked with comments, so the error seems obvious, but when you read such code with your eyes, it's very difficult to find such errors.
Let's look at another piece of code that is very difficult to perceive because it is similar and uninteresting.
MtpResponseCode MyMtpDatabase::getDevicePropertyValue(....) {
....
switch (type) {
case MTP_TYPE_INT8:
packet.putInt8(longValue);
break;
case MTP_TYPE_UINT8:
packet.putUInt8(longValue);
break;
case MTP_TYPE_INT16:
packet.putInt16(longValue);
break;
case MTP_TYPE_UINT16:
packet.putUInt16(longValue);
break;
case MTP_TYPE_INT32:
packet.putInt32(longValue);
break;
case MTP_TYPE_UINT32:
packet.putUInt32(longValue);
break;
case MTP_TYPE_INT64:
packet.putInt64(longValue);
break;
case MTP_TYPE_UINT64:
packet.putUInt64(longValue);
break;
case MTP_TYPE_INT128:
packet.putInt128(longValue);
break;
case MTP_TYPE_UINT128:
packet.putInt128(longValue); // <=
break;
....
}PVS-Studio warning: V525 CWE-682 The code contains the collection of similar blocks. Check items 'putInt8', 'putUInt8', 'putInt16', 'putUInt16', 'putInt32', 'putUInt32', 'putInt64', 'putUInt64', 'putInt128', 'putInt128' in lines 620, 623, 626, 629, 632, 635, 638, 641, 644, 647. android_mtp_MtpDatabase.cpp 620
In the case of MTP_TYPE_UINT128 the function putUInt128 had to be called instead of putInt128.
And the last example in this section is a gorgeous failed Copy-Paste.
static void btif_rc_upstreams_evt(....)
{
....
case AVRC_PDU_REQUEST_CONTINUATION_RSP: {
BTIF_TRACE_EVENT(
"%s() REQUEST CONTINUATION: target_pdu: 0x%02d",
__func__, pavrc_cmd->continu.target_pdu);
tAVRC_RESPONSE avrc_rsp;
if (p_dev->rc_connected == TRUE) {
memset(&(avrc_rsp.continu), 0, sizeof(tAVRC_NEXT_RSP));
avrc_rsp.continu.opcode =
opcode_from_pdu(AVRC_PDU_REQUEST_CONTINUATION_RSP);
avrc_rsp.continu.pdu = AVRC_PDU_REQUEST_CONTINUATION_RSP;
avrc_rsp.continu.status = AVRC_STS_NO_ERROR;
avrc_rsp.continu.target_pdu = pavrc_cmd->continu.target_pdu;
send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp);
}
} break;
case AVRC_PDU_ABORT_CONTINUATION_RSP: {
BTIF_TRACE_EVENT(
"%s() ABORT CONTINUATION: target_pdu: 0x%02d", __func__,
pavrc_cmd->abort.target_pdu);
tAVRC_RESPONSE avrc_rsp;
if (p_dev->rc_connected == TRUE) {
memset(&(avrc_rsp.abort), 0, sizeof(tAVRC_NEXT_RSP));
avrc_rsp.abort.opcode =
opcode_from_pdu(AVRC_PDU_ABORT_CONTINUATION_RSP);
avrc_rsp.abort.pdu = AVRC_PDU_ABORT_CONTINUATION_RSP;
avrc_rsp.abort.status = AVRC_STS_NO_ERROR;
avrc_rsp.abort.target_pdu = pavrc_cmd->continu.target_pdu;
send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp);
}
}
break;
....
}Before you read the analyzer warnings and further text, I suggest searching for the error yourself.
Here's a picture so that you don't accidentally read the answer. If you are interested in what is an egg inscribed with Java, then go here.
So, I hope you enjoyed searching for a typo. Now it is the time to cite the analyzer warning: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'abort' variable should be used instead of 'continu'. btif_rc.cc 1554
Apparently, the code was written by the Copy-Paste method, and a person, as always, was not able to be attentive while editing the copied code fragment. As a result, at the very end he did not replace "continu" with "abort".
I.e. in the second block the following should be written:
avrc_rsp.abort.target_pdu = pavrc_cmd->abort.target_pdu;This situation perfectly fits the definition of "The Last Line Effect", because the error occurred when changing the names in the last line.
A very funny bug related to the conversion between little-endian and big-endian data formats (see. Endianness).
inline uint32_t bswap32(uint32_t pData) {
return
(((pData & 0xFF000000) >> 24) | ((pData & 0x00FF0000) >> 8) |
((pData & 0x0000FF00) << 8) | ((pData & 0x000000FF) << 24));
}
bool ELFAttribute::merge(....) {
....
uint32_t subsection_length =
*reinterpret_cast<const uint32_t*>(subsection_data);
if (llvm::sys::IsLittleEndianHost !=
m_Config.targets().isLittleEndian())
bswap32(subsection_length);
....
}PVS-Studio warning: V530 CWE-252 The return value of function 'bswap32' is required to be utilized. ELFAttribute.cpp 84
There are no claims against the function bswap32. But its usage is incorrect.
bswap32(subsection_length);The author assumes that the variable is passed to the function by reference and gets changed there. However, he needs to use the value returned by the function. As a result, there is no data conversion.
The analyzer identified this bug as CWE-252: Unchecked Return Value. But, in fact, here it is more appropriate to call it CWE-198: Use of Incorrect Byte Ordering. Unfortunately, the analyzer cannot understand the meaning of an error here from the high-level perspective. However, this does not prevent it from revealing the serious defect in the code.
Correct code:
subsection_length = bswap32(subsection_length);In Android, there are 3 more places with the identical bug:
To avoid such errors, I can recommend using [[nodiscard]]. This attribute is used to indicate that the returned value of the function must be necessarily used when calling. Therefore, if you wrote like this:
[[nodiscard]] inline uint32_t bswap32(uint32_t pData) { ... }the error would be fixed at the stage of compiling the file. You can learn more details about some new useful attributes from the article of my colleague "C++17".
In programming and the theory of compilers, unreachable code is a part of a program that could not be performed under any circumstances because it is elusive in the control flow graph.
From the point of view of the Common Weakness Enumeration, it is CWE-561: Dead Code.
virtual sp<IEffect> createEffect(....)
{
....
if (pDesc == NULL) {
return effect;
if (status != NULL) {
*status = BAD_VALUE;
}
}
....
}PVS-Studio warning: V779 CWE-561 Unreachable code detected. It is possible that an error is present. IAudioFlinger.cpp 733
The operator return has to be lower along the code.
Other errors of this type:
Forgotten break inside of switch is a classic error of C and C++ programmers. To fight it, there appeared such a useful attribute in C++17, as [[fallthrough]]. More information about this error and [[fallthrough]] you can read in my article "break and fallthrough".
But while the world is full of old code where [[fallthrough]] is not used, you will need PVS-Studio. Let's look at a few bugs found in Android. According to the Common Weakness Enumeration, these errors are classified as CWE-484: Omitted Break Statement in Switch.
bool A2dpCodecConfigLdac::setCodecConfig(....) {
....
case BTAV_A2DP_CODEC_SAMPLE_RATE_192000:
if (sampleRate & A2DP_LDAC_SAMPLING_FREQ_192000) {
result_config_cie.sampleRate =
A2DP_LDAC_SAMPLING_FREQ_192000;
codec_capability_.sample_rate =
codec_user_config_.sample_rate;
codec_config_.sample_rate =
codec_user_config_.sample_rate;
}
case BTAV_A2DP_CODEC_SAMPLE_RATE_16000:
case BTAV_A2DP_CODEC_SAMPLE_RATE_24000:
case BTAV_A2DP_CODEC_SAMPLE_RATE_NONE:
codec_capability_.sample_rate =
BTAV_A2DP_CODEC_SAMPLE_RATE_NONE;
codec_config_.sample_rate =
BTAV_A2DP_CODEC_SAMPLE_RATE_NONE;
break;
....
}PVS-Studio warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. a2dp_vendor_ldac.cc 912
I think this error doesn't need explanation. I'd like to just note that this anomaly is revealed in code by more than just one way. For example, this error is also detected by the warnings V519:
Several similar bugs:
Return<void> EffectsFactory::getAllDescriptors(....) {
....
switch (status) {
case -ENOSYS: {
// Effect list has changed.
goto restart;
}
case -ENOENT: {
// No more effects available.
result.resize(i);
}
default: {
result.resize(0);
retval = Result::NOT_INITIALIZED;
}
}
....
}PVS-Studio warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. EffectsFactory.cpp 118
int Reverb_getParameter(....)
{
....
case REVERB_PARAM_REFLECTIONS_LEVEL:
*(uint16_t *)pValue = 0;
case REVERB_PARAM_REFLECTIONS_DELAY:
*(uint32_t *)pValue = 0;
case REVERB_PARAM_REVERB_DELAY:
*(uint32_t *)pValue = 0;
break;
....
}PVS-Studio warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. EffectReverb.cpp 1847
static SLresult IAndroidConfiguration_GetConfiguration(....)
{
....
switch (IObjectToObjectID((thiz)->mThis)) {
case SL_OBJECTID_AUDIORECORDER:
result = android_audioRecorder_getConfig(
(CAudioRecorder *) thiz->mThis, configKey,
pValueSize, pConfigValue);
break;
case SL_OBJECTID_AUDIOPLAYER:
result = android_audioPlayer_getConfig(
(CAudioPlayer *) thiz->mThis, configKey,
pValueSize, pConfigValue);
default:
result = SL_RESULT_FEATURE_UNSUPPORTED;
break;
}
....
}PVS-Studio warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. IAndroidConfiguration.cpp 90
Here I have collected errors related to incorrect memory management. Such warnings, according to the Common Weakness Enumeration, are classified as:
Let's start with functions that return a reference to an already destroyed variable.
TransformIterator& operator++(int) {
TransformIterator tmp(*this);
++*this;
return tmp;
}
TransformIterator& operator--(int) {
TransformIterator tmp(*this);
--*this;
return tmp;
}PVS-Studio warnings:
When the function finishes its execution, the variable tmp is destroyed, as it is created on the stack. Therefore, the functions return a reference to the already ruined (non-existent) object.
The correct solution would be to return by value:
TransformIterator operator++(int) {
TransformIterator tmp(*this);
++*this;
return tmp;
}
TransformIterator operator--(int) {
TransformIterator tmp(*this);
--*this;
return tmp;
}Let's see even more frustrating code, that deserves careful consideration.
int register_socket_transport(
int s, const char* serial, int port, int local)
{
atransport* t = new atransport();
if (!serial) {
char buf[32];
snprintf(buf, sizeof(buf), "T-%p", t);
serial = buf;
}
....
}PVS-Studio warning: V507 CWE-562 Pointer to local array 'buf' is stored outside the scope of this array. Such a pointer will become invalid. transport.cpp 1030
This is a dangerous piece of code. If the actual value of the argument serial is NULL, a temporary buffer on the stack must be used. When the body of the operator if is over, the array buf will cease to exist. The place where the buffer was created, can be used to store other variables that are created on the stack. There will be a hellish hodgepodge in data and consequences of such an error are barely predictable.
The following errors are related to incompatible ways of object creation and destruction.
void
SensorService::SensorEventConnection::reAllocateCacheLocked(....)
{
sensors_event_t *eventCache_new;
const int new_cache_size = computeMaxCacheSizeLocked();
eventCache_new = new sensors_event_t[new_cache_size];
....
delete mEventCache;
mEventCache = eventCache_new;
mCacheSize += count;
mMaxCacheSize = new_cache_size;
}PVS-Studio warning: V611 CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] mEventCache;'. Check lines: 391, 384. SensorEventConnection.cpp 391
It's all very simple here. The buffer, a pointer to which is stored in the class member mEventCache, is allocated using the operator new [], but this memory is released using the operator delete. It is wrong and it also leads to undefined behavior.
A similar error:
aaudio_result_t AAudioServiceEndpointCapture::open(....) {
....
delete mDistributionBuffer;
int distributionBufferSizeBytes =
getStreamInternal()->getFramesPerBurst() *
getStreamInternal()->getBytesPerFrame();
mDistributionBuffer = new uint8_t[distributionBufferSizeBytes];
....
}PVS-Studio warning: V611 CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] mDistributionBuffer;'. AAudioServiceEndpointCapture.cpp 50
I think that the error does not require explanation.
The following case is a bit more interesting, but the essence of an error is exactly the same.
struct HeifFrameInfo
{
....
void set(....) {
....
mIccData.reset(new uint8_t[iccSize]);
....
}
....
std::unique_ptr<uint8_t> mIccData;
};V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. HeifDecoderAPI.h 62
By default, smart pointer class std::unique_ptr calls delete for object destruction. However, in the function set, the memory is allocated by the operator new [].
Here is the correct variant:
std::unique_ptr<uint8_t[]> mIccData;Other errors:
This section will be finished by the errors related to memory leaks. Unpleasant surprise is that there are more than 20 of such errors. I think these are very painful defects leading to gradual reduction of free memory during long uptime of the operating system.
Asset* Asset::createFromUncompressedMap(FileMap* dataMap,
AccessMode mode)
{
_FileAsset* pAsset;
status_t result;
pAsset = new _FileAsset;
result = pAsset->openChunk(dataMap);
if (result != NO_ERROR)
return NULL;
pAsset->mAccessMode = mode;
return pAsset;
}PVS-Studio warning: V773 CWE-401 The function was exited without releasing the 'pAsset' pointer. A memory leak is possible. Asset.cpp 296
If it wasn't possible to open a chunk, the function would exit without destruction of an object, a pointer to which was stored in the variable pAsset. As a result, a memory leak will occur.
Other errors are identical, so I don't see any reason to consider them in the article. All wishing can see other warnings in the file: Android_V773.txt.
There is a large number of erroneous patterns, leading to "array index out of bounds" type of issues. In the case of Android, I identified only one erroneous pattern of the following type:
if (i < 0 || i > MAX)
return;
A[i] = x;In C and C++, array elements are numbered from 0, so the maximum index of the element in the array should be one less than the size of the array itself. The correct check should look as follows:
if (i < 0 || i >= MAX)
return;
A[i] = x;Array index out of bounds, according to the Common Weakness Enumeration is classified as CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer.
Let's see what these errors look like in the Android code.
static btif_hf_cb_t btif_hf_cb[BTA_AG_MAX_NUM_CLIENTS];
static bool IsSlcConnected(RawAddress* bd_addr) {
if (!bd_addr) {
LOG(WARNING) << __func__ << ": bd_addr is null";
return false;
}
int idx = btif_hf_idx_by_bdaddr(bd_addr);
if (idx < 0 || idx > BTA_AG_MAX_NUM_CLIENTS) {
LOG(WARNING) << __func__ << ": invalid index "
<< idx << " for " << *bd_addr;
return false;
}
return btif_hf_cb[idx].state ==
BTHF_CONNECTION_STATE_SLC_CONNECTED;
}PVS-Studio warning: V557 CWE-119 Array overrun is possible. The value of 'idx' index could reach 6. btif_hf.cc 277
Here is the correct version of the check:
if (idx < 0 || idx >= BTA_AG_MAX_NUM_CLIENTS) {There are two equal errors:
There are lots of ways to write an incorrectly working loop. In the Android code I met errors which, according to the Common Weakness Enumeration, can be classified as:
Of course, there are other ways to "shoot yourself in the foot" when writing loops, but this time I didn't come across them.
int main(int argc, char **argv)
{
....
char c;
printf("%s is already in *.base_fs format, just ....", ....);
rewind(blk_alloc_file);
while ((c = fgetc(blk_alloc_file)) != EOF) {
fputc(c, base_fs_file);
}
....
}PVS-Studio warning: V739 CWE-20 EOF should not be compared with a value of the 'char' type. The '(c = fgetc(blk_alloc_file))' should be of the 'int' type. blk_alloc_to_base_fs.c 61
The analyzer detected that the EOF constant is compared with a variable of the 'char' type. Let's figure out why this code is incorrect.
The function fgetc returns a value of the type int, namely, it can return a number from 0 to 255 or EOF (-1). The read value is placed into a variable of the char type. Because of that a symbol with the 0xFF (255) value turns into -1 and is handled in the same way as the file ending (EOF).
Users that use Extended ASCII Codes, may encounter a situation when one of the symbols of their alphabet is handled incorrectly by the program because of such errors. For example in the Windows-1251 code page, the last letter of Russian alphabet has the 0xFF code, and so is interpreted by some programs as the end-of-file character.
To summarize, we can say that the loop exit condition is written incorrectly. To fix this you need the variable c to have the int type.
Let's continue and consider more common errors when using the for operator.
status_t AudioPolicyManager::registerPolicyMixes(....)
{
....
for (size_t i = 0; i < mixes.size(); i++) {
....
for (size_t j = 0; i < mHwModules.size(); j++) { // <=
if (strcmp(AUDIO_HARDWARE_MODULE_ID_REMOTE_SUBMIX,
mHwModules[j]->mName) == 0
&& mHwModules[j]->mHandle != 0) {
rSubmixModule = mHwModules[j];
break;
}
....
}
....
}PVS-Studio warning: V534 CWE-691 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i'. AudioPolicyManager.cpp 2489
Because of a typo in the nested loop, in the condition the variable i is used although it is necessary to use the variable j. As a result, the variable j is incremented without any control, that eventually will lead to index out of bounds of the array mHwModules. It is impossible to predict what will happen next because the undefined behavior of a program will occur.
By the way, this fragment of code with an error was completely copied to another function. Therefore, the same exact error was found by the analyzer here: AudioPolicyManager.cpp 2586.
There are also 3 code fragments, which are very suspicious for me. However, I cannot claim that this code is exactly incorrect, since there is complex logic. In any case, I must draw your attention to this code so that the author checked it.
The first fragment:
void ce_t3t_handle_check_cmd(....) {
....
for (i = 0; i < p_cb->cur_cmd.num_blocks; i++) {
....
for (i = 0; i < T3T_MSG_NDEF_ATTR_INFO_SIZE; i++) {
checksum += p_temp[i];
}
....
}
....
}PVS-Studio warning: V535 CWE-691 The variable 'i' is being used for this loop and for the outer loop. Check lines: 398, 452. ce_t3t.cc 452
Note that the variable i is used for both external and internal loops.
Two more similar analyzer triggerings:
Are you tired yet? I suggest to pause for a while and download PVS-Studio to try to check your project.
Now let's move on.
#define NFA_HCI_LAST_PROP_GATE 0xFF
tNFA_HCI_DYN_GATE* nfa_hciu_alloc_gate(uint8_t gate_id,
tNFA_HANDLE app_handle) {
....
for (gate_id = NFA_HCI_FIRST_HOST_SPECIFIC_GENERIC_GATE;
gate_id <= NFA_HCI_LAST_PROP_GATE; gate_id++) {
if (gate_id == NFA_HCI_CONNECTIVITY_GATE) gate_id++;
if (nfa_hciu_find_gate_by_gid(gate_id) == NULL) break;
}
if (gate_id > NFA_HCI_LAST_PROP_GATE) {
LOG(ERROR) << StringPrintf(
"nfa_hci_alloc_gate - no free Gate ID: %u "
"App Handle: 0x%04x", gate_id, app_handle);
return (NULL);
}
....
}PVS-Studio warning: V654 CWE-834 The condition 'gate_id <= 0xFF' of loop is always true. nfa_hci_utils.cc 248
Please, note the following:
It turns out that the condition gate_id <= NFA_HCI_LAST_PROP_GATE is always true and cannot stop the loop execution.
The analyzer classified this error as CWE-834, but it can also be interpreted as CWE-571: Expression is Always True.
The following error in the loop is related to undefined behavior.
status_t SimpleDecodingSource::doRead(....) {
....
for (int retries = 0; ++retries; ) {
....
}PVS-Studio warning: V654 CWE-834 The condition '++ retries' of loop is always true. SimpleDecodingSource.cpp 226
Apparently, the programmer wanted the variable retries to take all possible values for the variable int and only after then the loop terminated.
The loop should stop when the expression ++retries is equal to 0. And this is only possible if the variable overflow occurs. As the variable is of a signed type, its overflow causes undefined behavior. Therefore, this code is incorrect and may lead to unpredictable consequences. For example, the compiler has a full right to remove the check and leave only the instruction for the counter increment.
And the last error in this section.
status_t Check(const std::string& source) {
....
int pass = 1;
....
do {
....
switch(rc) {
case 0:
SLOGI("Filesystem check completed OK");
return 0;
case 2:
SLOGE("Filesystem check failed (not a FAT filesystem)");
errno = ENODATA;
return -1;
case 4:
if (pass++ <= 3) {
SLOGW("Filesystem modified - rechecking (pass %d)",
pass);
continue; // <=
}
SLOGE("Failing check after too many rechecks");
errno = EIO;
return -1;
case 8:
SLOGE("Filesystem check failed (no filesystem)");
errno = ENODATA;
return -1;
default:
SLOGE("Filesystem check failed (unknown exit code %d)", rc);
errno = EIO;
return -1;
}
} while (0); // <=
return 0;
}PVS-Studio warning: V696 CWE-670 The 'continue' operator will terminate 'do { ... } while (FALSE)' loop because the condition is always false. Check lines: 105, 121. Vfat.cpp 105
Here is loop of such a kind:
do {
....
if (x) continue;
....
} while (0)To perform repeated operations, the programmer uses the operator continue. This is wrong. The operator continue does not resume the loop immediately but proceeds to check the conditions. As the condition is always false, the loop will be executed in any case only once.
The code should be rewritten in the following way to fix this error:
for (;;) {
....
if (x) continue;
....
break;
}A very common error is a repeated entry in the variable before the previous value is used. Most often these errors occur due to a typo or a failed Copy-Paste. According to the Common Weakness Enumeration, such errors are classified as CWE-563: Assignment to Variable without Use. Android has not been without such errors either.
status_t XMLNode::flatten_node(....) const
{
....
memset(&namespaceExt, 0, sizeof(namespaceExt));
if (mNamespacePrefix.size() > 0) {
namespaceExt.prefix.index =
htodl(strings.offsetForString(mNamespacePrefix));
} else {
namespaceExt.prefix.index = htodl((uint32_t)-1);
}
namespaceExt.prefix.index =
htodl(strings.offsetForString(mNamespacePrefix));
namespaceExt.uri.index =
htodl(strings.offsetForString(mNamespaceUri));
....
}PVS-Studio warning: V519 CWE-563 The 'namespaceExt.prefix.index' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1535, 1539. XMLNode.cpp 1539
Here's some pseudocode which highlights the essence of the error:
if (a > 0)
X = 1;
else
X = 2;
X = 1;Regardless of the condition, the variable X (in this case, it is namespaceExt.prefix.index) will always be set to a single value.
bool AudioFlinger::RecordThread::threadLoop()
{
....
size_t framesToRead = mBufferSize / mFrameSize;
framesToRead = min(mRsmpInFramesOA - rear, mRsmpInFramesP2 / 2);
....
}PVS-Studio warning: V519 CWE-563 The 'framesToRead' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 6341, 6342. Threads.cpp 6342
It was not clear why it was necessary to initialize a variable during the declaration if a different value was written immediately. Something is wrong here.
void SchedulingLatencyVisitorARM::VisitArrayGet(....) {
....
if (index->IsConstant()) {
last_visited_latency_ = kArmMemoryLoadLatency;
} else {
if (has_intermediate_address) {
} else {
last_visited_internal_latency_ += kArmIntegerOpLatency;
}
last_visited_internal_latency_ = kArmMemoryLoadLatency;
}
....
}PVS-Studio warning: V519 CWE-563 The 'last_visited_internal_latency_' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 680, 682. scheduler_arm.cc 682
Very strange, meaningless code. I would venture to guess that the following should have been written here:
last_visited_internal_latency_ += kArmMemoryLoadLatency;And the last error that demonstrates how tirelessly the analyzer finds errors that probably would be ignored even with careful code review.
void multiprecision_fast_mod(uint32_t* c, uint32_t* a) {
uint32_t U;
uint32_t V;
....
c[0] += U;
V = c[0] < U;
c[1] += V;
V = c[1] < V;
c[2] += V; //
V = c[2] < V; // <=
c[2] += U; //
V = c[2] < U; // <=
c[3] += V;
V = c[3] < V;
c[4] += V;
V = c[4] < V;
c[5] += V;
V = c[5] < V;
....
}PVS-Studio warning: V519 CWE-563 The 'V' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 307, 309. p_256_multprecision.cc 309
The code is so "red-eye", that I don't want to deal with it. While it is obvious that here a typo takes place in code, which I highlighted with comments.
Only various errors remained, for which there is no point to make special sections. However, they are equally interesting and insidious, as the ones discussed earlier.
Precedence of operators
void TagMonitor::parseTagsToMonitor(String8 tagNames) {
std::lock_guard<std::mutex> lock(mMonitorMutex);
// Expand shorthands
if (ssize_t idx = tagNames.find("3a") != -1) {
ssize_t end = tagNames.find(",", idx);
char* start = tagNames.lockBuffer(tagNames.size());
start[idx] = '\0';
....
}
....
}PVS-Studio warning: V593 CWE-783 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. TagMonitor.cpp 50
According to the Common Weakness Enumeration classification: CWE-783: Operator Precedence Logic Error.
A programmer conceived the following. There is a search of a substring "3a" and the position of this substring is written in the variable idx. If the substring is found (idx != -1), then the code, in which the value of the variable idx is used, starts to run.
Unfortunately, the programmer confused priorities of operations. Actually the check is working as follows:
if (ssize_t idx = (tagNames.find("3a") != -1))First it is checked if there is a substring "3a" in the string and the result (false or true) is placed in the variable idx. As a result, the variable idx has the value 0 or 1.
If the condition is true (variable idx is equal to 1), then the logic that uses the variable idx begins to execute. The variable that is always equal to 1 will lead to incorrect program behavior.
One can fix the error by setting the initialization of the variable out from the condition:
ssize_t idx = tagNames.find("3a");
if (idx != -1)The new version of C++17 also allows you to write:
if (ssize_t idx = tagNames.find("3a"); idx != -1)Wrong constructor
struct HearingDevice {
....
HearingDevice() { HearingDevice(RawAddress::kEmpty, false); }
....
};PVS-Studio warning: V603 CWE-665 The object was created but it is not being used. If you wish to call constructor, 'this->HearingDevice::HearingDevice(....)' should be used. hearing_aid.cc 176
According to the classification of Common Weakness Enumeration: CWE-665: Improper Initialization.
Programmers often make mistakes, trying to explicitly call the constructor to initialize the object. In the class there are two constructors. To reduce the size of the source code a programmer decided to call one constructor from another. But this code does not do what the programmer expects.
The following occurs. A new unnamed object of the type HearingDevice is created and immediately destroyed. As a result, the class fields are left uninitialized.
To correct this error, you can use the delegate constructor (this feature appeared in C++11). Correct code:
HearingDevice() : HearingDevice(RawAddress::kEmpty, false) { }The function does not return a value
int NET_RecvFrom(int s, void *buf, int len, unsigned int flags,
struct sockaddr *from, int *fromlen) {
socklen_t socklen = *fromlen;
BLOCKING_IO_RETURN_INT(
s, recvfrom(s, buf, len, flags, from, &socklen) );
*fromlen = socklen;
}PVS-Studio warning: V591 CWE-393 Non-void function should return a value. linux_close.cpp 139
According to the classification of Common Weakness Enumeration: CWE-393: Return of Wrong Status Code.
The function returns a random value. One more similar bug: V591 CWE-393 Non-void function should return a value. linux_close.cpp 158
Incorrect evaluation of the structure size
int MtpFfsHandle::handleControlRequest(....) {
....
struct mtp_device_status *st =
reinterpret_cast<struct mtp_device_status*>(buf.data());
st->wLength = htole16(sizeof(st));
....
}PVS-Studio warning: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'st' class object. MtpFfsHandle.cpp 251
I'm sure that a developer wanted to place the structure size but not the pointer size in the member variable wLength. Most likely, the correct code should be like this:
st->wLength = htole16(sizeof(*st));Similar analyzer triggerings:
Pointless bit operations
#define EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR 0x00000001
#define EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR 0x00000002
#define EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR 0x00000004
EGLContext eglCreateContext(....)
{
....
case EGL_CONTEXT_FLAGS_KHR:
if ((attrib_val | EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) ||
(attrib_val | EGL_CONTEXT_OPENGL_FORWARD_C....) ||
(attrib_val | EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR))
{
context_flags = attrib_val;
} else {
RETURN_ERROR(EGL_NO_CONTEXT,EGL_BAD_ATTRIBUTE);
}
....
}PVS-Studio warning: V617 CWE-480 Consider inspecting the condition. The '0x00000001' argument of the '|' bitwise operation contains a non-zero value. egl.cpp 1329
According to the classification of Common Weakness Enumeration: CWE-480: Use of Incorrect Operator.
The expression (A | 1) || (A | 2) || (A | 4) is meaningless and as the result it will always be true. In fact, one should use the operator &, and then the code becomes meaningful:
if ((attrib_val & EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) ||
(attrib_val & EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR) ||
(attrib_val & EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR))A similar error: V617 CWE-480 Consider inspecting the condition. The '0x00000001' argument of the '|' bitwise operation contains a non-zero value. egl.cpp 1338
Incorrect bit shift
template <typename AddressType>
struct RegsInfo {
....
uint64_t saved_reg_map = 0;
AddressType saved_regs[64];
....
inline AddressType* Save(uint32_t reg) {
if (reg > sizeof(saved_regs) / sizeof(AddressType)) {
abort();
}
saved_reg_map |= 1 << reg;
saved_regs[reg] = (*regs)[reg];
return &(*regs)[reg];
}
....
}PVS-Studio warning: V629 CWE-190 Consider inspecting the '1 << reg' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. RegsInfo.h 47
According to the classification of the Common Weakness Enumeration: CWE-190: Integer Overflow or Wraparound.
When shifting 1 << reg, the value of the variable reg lies in the range [0..63]. The expression serves to receive various powers of 2, beginning with 2^0 and ending with 2^63.
The code does not work. The fact of the matter is that the numeric literal 1 has a 32-bit type int. So it will not be possible to get the value greater than 1^31. The shift for a higher value will result in an overflow of the variable and the emergence of undefined behavior.
Correct code:
saved_reg_map |= static_cast<uint64_t>(1) << reg;or:
saved_reg_map |= 1ULL << reg;Strings are copied in themselves
void PCLmGenerator::writeJobTicket() {
// Write JobTicket
char inputBin[256];
char outputBin[256];
if (!m_pPCLmSSettings) {
return;
}
getInputBinString(m_pPCLmSSettings->userInputBin, &inputBin[0]);
getOutputBin(m_pPCLmSSettings->userOutputBin, &outputBin[0]);
strcpy(inputBin, inputBin);
strcpy(outputBin, outputBin);
....
}PVS-Studio warnings:
According to the classification of the Common Weakness Enumeration: CWE-688: Function Call With Incorrect Variable or Reference as Argument.
Strings are copied in themselves for some reason. Most likely, some typos are made here.
Use of uninitialized variable
void mca_set_cfg_by_tbl(....) {
tMCA_DCB* p_dcb;
const tL2CAP_FCR_OPTS* p_opt;
tMCA_FCS_OPT fcs = MCA_FCS_NONE;
if (p_tbl->tcid == MCA_CTRL_TCID) {
p_opt = &mca_l2c_fcr_opts_def;
} else {
p_dcb = mca_dcb_by_hdl(p_tbl->cb_idx);
if (p_dcb) {
p_opt = &p_dcb->p_chnl_cfg->fcr_opt;
fcs = p_dcb->p_chnl_cfg->fcs;
}
}
memset(p_cfg, 0, sizeof(tL2CAP_CFG_INFO));
p_cfg->mtu_present = true;
p_cfg->mtu = p_tbl->my_mtu;
p_cfg->fcr_present = true;
memcpy(&p_cfg->fcr, p_opt, sizeof(tL2CAP_FCR_OPTS)); // <=
....
}PVS-Studio warning: V614 CWE-824 Potentially uninitialized pointer 'p_opt' used. Consider checking the second actual argument of the 'memcpy' function. mca_main.cc 252
According to the classification of the Common Weakness Enumeration: CWE-824: Access of Uninitialized Pointer.
If p_tbl->tcid != MCA_CTRL_TCID and p_dcb == nullptr, then the pointer p_opt remains uninitialized.
Strange use of an uninitialized variable
struct timespec
{
__time_t tv_sec; /* Seconds. */
long int tv_nsec; /* Nanoseconds. */
};
static inline timespec NsToTimespec(int64_t ns) {
timespec t;
int32_t remainder;
t.tv_sec = ns / kNanosPerSecond;
remainder = ns % kNanosPerSecond;
if (remainder < 0) {
t.tv_nsec--;
remainder += kNanosPerSecond;
}
t.tv_nsec = remainder;
return t;
}PVS-Studio warning: V614 CWE-457 Uninitialized variable 't.tv_nsec' used. clock_ns.h 55
According to the classification of the Common Weakness Enumeration: CWE-457: Use of Uninitialized Variable.
At the time of the variable t.tv_nsec decrement, it is uninitialized. The variable is initialized later: t.tv_nsec = remainder;. Something is obviously messed up here.
Redundant expression
void bta_dm_co_ble_io_req(....)
{
....
*p_auth_req = bte_appl_cfg.ble_auth_req |
(bte_appl_cfg.ble_auth_req & 0x04) |
((*p_auth_req) & 0x04);
....
}PVS-Studio warning: V578 An odd bitwise operation detected. Consider verifying it. bta_dm_co.cc 259
The expression is redundant. If you delete the subexpression (bte_appl_cfg.ble_auth_req & 0x04), then the result of the expression will not change. Perhaps there is some sort of a typo.
Leak of a descriptor
bool RSReflectionCpp::genEncodedBitCode() {
FILE *pfin = fopen(mBitCodeFilePath.c_str(), "rb");
if (pfin == nullptr) {
fprintf(stderr, "Error: could not read file %s\n",
mBitCodeFilePath.c_str());
return false;
}
unsigned char buf[16];
int read_length;
mOut.indent() << "static const unsigned char __txt[] =";
mOut.startBlock();
while ((read_length = fread(buf, 1, sizeof(buf), pfin)) > 0) {
mOut.indent();
for (int i = 0; i < read_length; i++) {
char buf2[16];
snprintf(buf2, sizeof(buf2), "0x%02x,", buf[i]);
mOut << buf2;
}
mOut << "\n";
}
mOut.endBlock(true);
mOut << "\n";
return true;
}PVS-Studio warning: V773 CWE-401 The function was exited without releasing the 'pfin' handle. A resource leak is possible. slang_rs_reflection_cpp.cpp 448
The analyzer classified this error, according to Common Weakness Enumeration, as: CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak'). However, it would be more correct to issue the CWE-775: Missing Release of File Descriptor or Handle after Effective Lifetime. I'll ask my colleagues to fix this underworking in PVS-Studio.
The descriptor is released nowhere. A developer simply forgot to call the function fclose in the end. A nasty error that can quickly consume the entire stock of available descriptors, then it will be impossible to open new files.
As you can see, even in such a famous and well tested project, as Android, the PVS-Studio analyzer easily detects many errors and potential vulnerabilities. Let's now summarize what weaknesses (potential vulnerabilities) were found:
In total, I cited descriptions of 490 weaknesses in the article. In fact, the analyzer can identify even more of them, but as I wrote earlier, I have not found enough energy to have a closer look at the report.
The size of the checked code base is approximately 2168000 lines of C and C++ code. 14.4% of them are comments. Totally we receive about 1855000 strings of pure code.
Thus, we have 490 CWE for 1855000 lines of code.
It turns out that the PVS-Studio analyzer is able to detect more than 1 weakness (a potential vulnerability) for every 4000 lines of code in the Android project. A good result for the code analyzer, I'm glad.
Thank you for your attention. I wish all bugless code and suggest to do the following: