Checked with the help of PVS-Studio Android source codes, or no one is perfect

The development of large complex projects is impossible without the use of programming methodologies and tools that help control the quality of the code. First of all, it is a competent coding standard, code reviews, unit tests, static and dynamic code analyzers. All this helps to identify defects in the code at the earliest stages of development. This article demonstrates the capabilities of the PVS-Studio static analyzer to detect errors and potential vulnerabilities in the code of the Android operating system. We hope that the article will attract the attention of readers to the methodology of static code analysis and they will want to implement it in the process of developing their own projects.

Introduction

A year has passed since the writing of a large article about errors in the Tizen operating system, and I wanted to again conduct an equally interesting study of some operating system. The choice fell on Android.

The code of the Android operating system is of high quality, well tested. During development, at least a static code analyzer is used for the Coverity code, as evidenced by comments like this:

/* Coverity: [FALSE-POSITIVE error] intended fall through *//* Missing break statement between cases in switch statement *//* fall through */

In general, this is an interesting, high-quality project, and to find errors in it is a challenge for our static analyzer PVS-Studio.

I think, only by the volume of the article, the reader understands that the PVS-Studio analyzer coped with the task perfectly and found a lot of defects in the code of this operating system.

Common Weakness Enumeration

In the article you will find references to the Common Weakness Enumeration (CWE). We explain the reason for sending this list and why it is important from a security point of view.

Often the cause of vulnerabilities in programs is not some tricky combination of circumstances, but a banal program error. It would be appropriate here to quote this from the website of prqa.com :

The National Institute for Standards and Technology (NIST) reports.

You can read the article " How can PVS-Studio help in finding vulnerabilities? " With some examples of simple errors that led to vulnerabilities in projects such as MySQL, iOS, NAS, illumos-gate.

Accordingly, many vulnerabilities can be eliminated by detecting common errors in time and correcting them. And here comes the Common Weakness Enumeration.

Errors are different, and not all errors are dangerous in terms of security. Those errors that could potentially cause vulnerabilities are collected in Common Weakness Enumeration. This list is replenished, and for sure there are errors that can lead to vulnerabilities, but they have not yet been included in this list.

However, if the error is classified according to the CWE, then it is theoretically possible that it can be exploited as a vulnerability ( CVE). Yes, the probability of this is small. Very rarely does CWE turn into CVE. However, if you want to protect your code from vulnerabilities, you should, if possible, find as many of the errors described in CWE as possible and fix them.

The schematic relationship between PVS-Studio, errors, CWE and CVE is shown in the figure:

Part of the error is classified as CWE. PVS-Studio can detect many of these errors, thereby preventing some of these defects from becoming vulnerabilities (CVE).

It can be said that PVS-Studio reveals many potential vulnerabilities before they caused harm. Thus, PVS-Studio is a tool for static application security testing ( SAST ).

Now, I think, it is clear why, in describing errors, I found it important to note how they are classified according to CWE. This makes it easier to demonstrate the importance of using a static code analyzer in responsible projects, which are uniquely related to operating systems.

Android check

To analyze the project, I used the PVS-Studio analyzer version 6.24. The analyzer currently supports the following languages ​​and compilers:

• Windows Visual Studio 2010-2017 C, C ++, C ++ / CLI, C ++ / CX (WinRT), C #
• Windows IAR Embedded Workbench, C / C ++ Compiler for ARM C, C ++
• Windows / Linux. Keil µVision, DS-MDK, ARM Compiler 5/6 C, C ++
• Windows / Linux. Texas Instruments Code Composer Studio, ARM Code Generation Tools C, C ++
• Windows / Linux / macOS. Clang C, C ++
• Linux / macOS. GCC C, C ++
• Windows MinGW C, C ++

Note. Perhaps some of our readers missed the news that we supported the work in the macOS environment, and they will be interested in this publication: " The release of PVS-Studio for macOS: 64 weaknesses in the Apple XNU Kernel ".

The process of checking the Android source codes was not a problem, so I will not dwell on it. The problem, rather, was my busyness with other tasks, which is why I did not find the time and strength to look at the report as closely as I would like. However, even a cursory review turned out to be more than enough to collect a large collection of interesting errors for a solid article.

The most important thing: I ask Android developers not only to correct the errors described in the article, but also to conduct a more careful independent analysis. I looked at the analyzer report superficially and could miss a lot of serious errors.

When you first check the analyzer will give a lot of 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 required. In general, write to us , we will help and advise.

Now let's see what errors and potential vulnerabilities I managed to find. I hope you will like what the PVS-Studio static code analyzer can find. Enjoy reading.

Meaningless comparisons

The analyzer considers expressions abnormal if they are always true or false. Such warnings, according to Common Weakness Enumeration, are classified as:

• CWE-570 : Expression is Always False
• CWE-571 : Expression is Always True

The analyzer generates many such warnings, and, unfortunately, most of them are false for Android code. In this case, the analyzer is not to blame. Just the code is written like this.
I will show it on a simple example.

#if GENERIC_TARGETconstchar alternative_config_path[] = "/data/nfc/";
#elseconstchar 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 fact is that the GENERIC_TARGET macro is not defined, and from the analyzer’s point of view, the code looks like this:

constchar alternative_config_path[] = "";
....
if (alternative_config_path[0] != '\0') {

The analyzer is simply obliged to issue a warning, since the string is empty, and at zero offset there is always a terminal zero. Thus, the analyzer is formally right, issuing a warning. However, from a practical point of view, there is no benefit from this warning.

With such situations, unfortunately, nothing can be done. You will have to methodically review all such warnings and mark many places as false positives, so that the analyzer will no longer issue a message code for these lines. This really needs to be done, because, in addition to meaningless messages, a lot of these defects will be found.

I admit honestly that I was not interested in carefully reviewing warnings of this type, and I ran through them superficially. However, even this will be enough to show that such diagnostics are very useful and find interesting errors.

I want to start with the classic situation, when the function of comparing two objects is incorrectly implemented. Why classic? This is a typical error pattern that is constantly found in various projects. Most likely, there are three reasons for its occurrence:

1. The comparison functions are simple and are written “on the machine” and using the Copy-Paste technology. A person when writing such a code is inconsiderate and often makes typos.
2. Usually, code-review is not performed for such functions, as it is too lazy to watch simple and boring functions.
3. For such functions, unit tests are usually not done. Because laziness. In addition, the functions are simple, and programmers do not think that errors are possible in them.

These thoughts and examples are described in more detail in the article “ Evil lives in comparison functions ”.

staticinlineboolisAudioPlaybackRateEqual(
  const AudioPlaybackRate &pr1,
  const AudioPlaybackRate &pr2){
  returnfabs(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, we have a classic function of comparing two objects of type AudioPlaybackRate . And, as I think, the reader realizes that it is wrong. The PVS-Studio analyzer notes here two typos at once:

• V501 CWE-571 operator == 'operator: pr2.mStretchMode == pr2.mStretchMode AudioResamplerPublic.h 107
• V501 CWE-571 operator: pr2.mFallbackMode == pr2.mFallbackMode AudioResamplerPublic.h 108

The pr2.mStretchMode field and the pr2.mFallbackMode field are compared to themselves. It turns out that the function compares objects with insufficient accuracy.

The following meaningless comparison lives in a function that stores fingerprint information in a file.

staticvoidsaveFingerprint(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;
}

Anomaly is detected in this code at once by two diagnostics:

• V501 CWE-570 ||| operator: ns! = 1 || ns! = 1 fingerprint.c 126
• V560 CWE-570 A part of conditional expression is always false: ns! = 1. fingerprint.c 126

There is no handling of the situation when the second call to the fwrite function cannot write data to the file. In other words, the value of the variable nf is not checked . The correct check should look like this:

if (ns != 1 || nf != 1)

We proceed to the next error associated with the use of the operator & .

#define O_RDONLY 00000000#define O_WRONLY 00000001#define O_RDWR   00000002static 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 the conditional expression is always false: f-> mode & 00000000. fec_read.cpp 322

Notice that the O_RDONLY constant is zero. This makes the expression f-> mode & O_RDONLY meaningless, since it is always equal to 0. It turns out that the condition of the if statement is never executed, and statement-true is a dead code.

The correct check should be:

if (f->mode == O_RDONLY && expect_zeros) {

Now let's consider the classic typo when you forgot to write 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, since the operand InputReaderConfiguration :: CHANGE_DISPLAY_INFO is a constant equal to 4.

If you look at how the code is written next door, it becomes clear that in fact the condition should be like this:

if (!changes ||
    (changes & InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {

The following comparison, which does not make sense, I met in the statement of the cycle.

voidparse_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);
    } elseif (strcmp("....", ippGetName(attrptr)) == 0) {
      ....BottomMargin = ippGetInteger(attrptr, 0);
    } elseif (strcmp("....", ippGetName(attrptr)) == 0) {
      ....LeftMargin = ippGetInteger(attrptr, 0);
    } elseif (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

Notice that the value of the variable j is not incremented anywhere. This means that the subexpression (j <4) is always true.

The greatest number of useful operations of the PVS-Studio analyzer, which always refer to true / false conditions, refers to the code where the result of creating objects is checked using the new operator . In other words, the analyzer detects the following code pattern:

T *p = new T;
if (p == nullptr)
  return ERROR;

Such checks are meaningless. If it was not possible to allocate memory for the object, then an exception of the type std :: bad_alloc is generated, and the matter simply does not get to check the pointer value.

Note. Operator new can return nullptr , if you write a new (the std :: nothrow) T . However, this does not apply to the discussed errors. The PVS-Studio analyzer takes into account (std :: nothrow) and does not issue a warning if an object is created in this way.

It may seem that such errors are harmless. Well, think, an extra check that never works. Anyway, after all, an exception is generated that will be processed somewhere. Unfortunately, some developers place operator statement-trueif actions releasing resources, etc. Since this code is not executed, it can lead to memory leaks and other errors.

Consider one of these cases that I have noticed in the Android code.

intparse_apk(constchar *path, constchar *target_package_name){
  ....
  FileMap *dataMap = zip->createEntryFileMap(entry);
  if (dataMap == NULL) {
    ALOGW("%s: failed to create FileMap\n", __FUNCTION__);
    return-1;
  }
  char *buf = newchar[uncompLen];
  if (NULL == buf) {
    ALOGW("%s: failed to allocate %" PRIu32 " byte\n",
          __FUNCTION__, uncompLen);
    delete dataMap;
    return-1;
  }
  ....
}

PVS-Studio warning: V668 CWE-570 has been defined as the “new” operator. The exception will be generated in the case of memory allocation error. scan.cpp 213

Note that if it is impossible to allocate a second memory block, the programmer tries to free the first block:

delete dataMap;

This code will never get control. This is a dead code. If an exception occurs, a memory leak will occur.

Writing such code is fundamentally wrong. For such cases, smart pointers were invented .

In total, the PVS-Studio analyzer found in the Android code 176 places where the pointer is checked after creating objects with new . I did not begin to understand how dangerous each of these places was, and, of course, I would not overload the article with all these warnings. Those interested can see other warnings in the file Android_V668.txt .

Null pointer dereferencing

Dereferencing a null pointer leads to undefined program behavior, so it is 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:

• CWE-119 : Improve Restriction of Memory Buffer
• CWE-476 : NULL Pointer Dereference
• CWE-628 : Function Call with Incorrectly Specified Arguments
• CWE-690 : Unchecked Return Value to NULL Pointer Dereference

I often find such errors in code that is responsible for handling non-standard or incorrect situations. Nobody tests such code, and the error can live in it for a very long time. Now will be considered just such a case.

boolparseEffect(....){
  ....
  if (xmlProxyLib == nullptr) {
    ALOGE("effectProxy must contain a <%s>: %s",
          tag, dump(*xmlProxyLib));
    returnfalse;
  }
  ....
}

PVS-Studio warning: V522 CWE-476 Dereferencing of the null pointer 'xmlProxyLib' might take place. EffectsConfig.cpp 205

If the xmlProxyLib pointer is nullptr , the programmer displays a debugging message, which requires that this pointer be dereferenced. Oops ...

Now let's look at a more interesting version of the error.

staticvoidsoinfo_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;
  } elseif (local_unload_list.contains(needed)) {
    return;
  } elseif (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 is needed! = nullptr , then a warning is output, which is a very suspicious program behavior. Finally, it becomes clear that the code contains an error, if you look below and see that when needed == nullptr, a null pointer dereference will occur in the expression needed-> is_linked () .

Most likely, the operators are simply confused here! = And ==. If you make a replacement, the function code makes sense, and the error disappears.

The bulk of warnings about a potential null pointer dereference refers to a situation like:

T *p = (T *) malloc (N);
*p = x;

Functions like malloc , strdup, and so on can return NULL if memory cannot be allocated. Therefore, you cannot dereference pointers that return these functions without first checking the pointer.

There are a lot of similar errors, so I will give 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 a null pointer 'param'. Check lines: 245, 244. BufferProviders.cpp 245

staticchar* descriptorClassToDot(constchar* str){
  ....
  newStr = strdup(lastSlash);
  newStr[strlen(lastSlash)-1] = '\0';
  ....
}

PVS-Studio warning: V522 CWE-690 There might be a null pointer 'newStr'. Check lines: 203, 202. DexDump.cpp 203

Someone can say that these are minor errors. If there is not enough memory, the program will simply crash when dereferencing the null pointer, and this is normal. Since there is no memory, then there is nothing to try to somehow handle this situation.

Such a person is wrong. Pointers must be checked! I analyzed this topic in detail in the article “ Why it is important to check that the malloc function returned ”. I highly recommend reading it to anyone who has not read it yet.

In short, the danger is that writing to the memory may not necessarily occur near the zero address. You can write data somewhere very far into a non-write protected memory page, and thus cause a subtle error or you can start using this error as a vulnerability. Let's see what I mean by the example of the check_size function .

intcheck_size(radio_metadata_buffer_t **metadata_ptr,
               constuint32_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 a case, it will not be used. Check lines: 91, 89. radio_metadata.c 91

I did not understand the logic of the function, but this is not necessary. The main thing is that a new buffer is created and the data is copied there. If the realloc function returns NULL , then the data will be copied to the address (((uint32_t *) NULL + metadata-> size_int - (metadata-> count + 1)).

If the metadata-> size_int value is large, then the consequences will be regrettable. It turns out that the data is written to a random memory location.

By the way, there is another kind of null pointer dereference, which the PVS-Studio analyzer classifies not as CWE-690, but as CWE-628 (incorrect argument).

staticvoidparse_tcp_ports(constchar *portstring, uint16_t *ports){
  char *buffer;
  char *cp;
  buffer = strdup(portstring);
  if ((cp = strchr(buffer, ':')) == NULL)
  ....
}

PVS-Studio warning: V575 CWE-628 The potential of the null pointer has passed to 'strchr' function. Inspect the first argument. Check lines: 47, 46. libxt_tcp.c 47

The fact is that pointer dereference will occur when the strchr function is called . Therefore, the analyzer interprets this situation as a transfer to the function of an incorrect value.

The remaining 194 warnings of this type I list in the file Android_V522_V575.txt .

By the way, a special piquancy to all these errors is given by the warnings considered earlier about checking the pointer after calling the new operator . It turns out that there are 195 function calls malloc / realloc / strdupand so on, when the pointer is not checked. But there are 176 places where the pointer is checked after calling new . Agree, a weird approach!

Finally, it remains for us to consider the warnings V595 and V1004, which are also associated with the use of null pointers.

V595 detects situations in which the pointer is dereferenced and then checked. Synthetic example:

p->foo();
if (!p) Error();

V1004 reveals the opposite situation, when first the pointer was checked, and then forgot to do it. Synthetic example:

if (p) p->foo();
p->doo();

Let's look at a few fragments of Android code, where there are errors of this type. Specially explain their features is not required.

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 Check lines: 385, 388. rate_control.cpp 385

staticvoidresampler_reset(struct resampler_itfe *resampler){
  structresampler *rsmp = (structresampler *)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' Check lines: 54, 57. resampler.c 54

voidbta_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

Considering other warnings of this type is not interesting. Among them there are both errors and false warnings that arise because of poorly or difficultly written code.

I wrote a dozen useful warnings:

• V1004 CWE-476 The 'ain' pointer was used unsafely after it was verified against nullptr. Check lines: 101, 105. rsCpuIntrinsicBLAS.cpp 105
• V595 CWE-476 The 'outError' pointer was utilized before it was verified against nullptr. Check lines: 437, 450. Command.cpp 437
• V595 CWE-476 The 'out_last_reference' pointer was utilized before it was verified against nullptr. Check lines: 432, 436. AssetManager2.cpp 432
• V595 CWE-476 The 'set' pointer was utilized before it was verified against nullptr. Check lines: 4524, 4529. ResourceTypes.cpp 4524
• V595 CWE-476 The 'reply' pointer was utilized before it was verified against nullptr. Check lines: 126, 133. Binder.cpp 126
• V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 532, 540. rate_control.cpp 532
• V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 702, 711. rate_control.cpp 702
• V595 CWE-476 The 'pInfo' pointer was used before it was verified against nullptr. Check lines: 251, 254. ResolveInfo.cpp 251
• V595 CWE-476 The 'address' pointer was used before it was verified against nullptr. Check lines: 53, 55. DeviceHalHidl.cpp 53
• V595 CWE-476 The 'halAddress' pointer was used before it was verified against nullptr. Check lines: 55, 82. DeviceHalHidl.cpp 55

And then I got bored, and I filtered this type of warning. Therefore, I do not even know how many real errors were detected by the analyzer. These warnings are waiting for their hero, who will carefully examine them and make changes to the code.

I just want to draw the attention of my new readers to errors like this:

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 Check lines: 686, 689. ndapi.c 686

Some people think that there is no error here, since “there is no real pointer dereference”. The address of a non-existent variable is simply calculated. Further, if the pointer iwnn is zero, then the function will exit. Consequently, nothing bad happened that we previously incorrectly calculated the address of a member of the class.

No, you can not talk like that. This code leads to undefined behavior, and therefore it is impossible to write like this. Undefined behavior can manifest itself, for example, as follows:

1. The compiler sees that the pointer is dereferenced: iwnn-> previous_selection
2. You cannot dereference a null pointer, because this is an undefined behavior.
3. The compiler infers that the iwnn pointer is always non-zero.
4. The compiler removes the extra check: if (iwnn == NULL)
5. Everything, now when executing the program, a check for a null pointer is not performed, and work begins with an incorrect pointer to a class member

This topic is described in more detail in my article " Dereferencing a null pointer leads to undefined behavior ."

Private data is not overwritten

Consider a serious view of a potential vulnerability that is classified according to Common Weakness Enumeration as CWE-14 : Compiler Removal.

In short, the point is this: the compiler has the right to remove the memset function call , if after that the buffer is no longer used.

When I write about this type of vulnerability, comments will surely appear that this is just a glitch in the compiler that needs to be fixed. No, this is not a glitch. And before you object, please read the following materials:

• Roman Fomichev. Secure cleaning of private data .
• Common Weakness Enumeration. CWE-14 .
• Description of the V597 diagnostic .

In general, everything is serious. Are there any bugs in Android? Of course have. They are generally many where there is: proof :).

Let's go back to the Android code and look at the beginning and end of the FwdLockGlue_InitializeRoundKeys function , since we are not interested in the middle.

staticvoidFwdLockGlue_InitializeRoundKeys(){
  unsignedchar 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 keyEncryptionKey array is created on the stack and stores private information. At the end of the function, they want to fill this array with zeros, so that it does not accidentally go anywhere. How information can get where it does not follow, will tell the article " Overwrite memory - why? ".

To fill an array with private information with zeros, the memset function is used . The comment “Zero out key data” confirms that we understand everything correctly.

The trouble is that with very high probability the compiler when assembling the release-version will remove the memset function call . Since the buffer after the memset call is not used, the call to the memset function itself is superfluous from the point of view of the compiler.

More 10 warnings I wrote a file Android_V597.txt .

There was another error where the memory is not overwritten, although in this case the memset function has nothing to do with it.

voidSHA1Transform(uint32_t state[5], constuint8_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. sha1.c 213

PVS-Studio revealed an anomaly due to the fact that after assigning variables to values, they are no longer used. The analyzer has classified this defect as CWE-563 : Assignment to Variable without Use. And, formally, he is right, although, in fact, here we are again dealing with CWE-14. The compiler will discard these assignments, since from the point of view of C and C ++ they are redundant. As a result, the previous values ​​of the variables a , b , c , d and e that store private data will remain in the stack .

Unspecified / implementation-defined behavior

While you are not tired, let's consider a complex case that will require a thorough description on my part.

typedefint32_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’s 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

The programmer wanted to check that the 8 high bits of the variable d contain ones, but not all the bits at once. In other words, the programmer wanted to verify that any value other than 0x00 and 0xFF is in the high byte.

He approached this task too creatively. For a start, he checked that the high-order bits are non-zero by writing the expression (d >> 24). There are complaints about this expression, but it is more interesting to analyze the right side of the expression: ((d >> 24) +1). The programmer shifts the high eight bits to the low byte. At the same time, he expects that the most significant sign bit is duplicated in all other bits. Those. if the variable d is 0b11111111'00000000'00000000'00000000, then after the shift the value will be 0b11111111'1111111111111111'11111111. Adding 1 to the value 0xFFFFFFFF of type int , the programmer plans to get 0. That is: -1 + 1 = 0. Thus, by the expression ((d >> 24) +1), he checks that not all of the upper eight bits are equal to 1. I understand that it is rather difficult,

Now let's see what is wrong with this code.

When shifting, the leading 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. It is an insignia of the E1 / 2 ^ E2. If E1 has a signed value

We care about the latest offer. So, we met the behavior defined by the implementation (implementation-defined behavior). How this code will work depends on the microprocessor architecture and the compiler implementation. After a shift in the high-order bits, zeros may well turn out, and then the expression ((d >> 24) +1) will always be different from 0, i.e. will always be true value.

Hence the conclusion: do not be fooled in vain. The code will become more reliable and clearer if you write, for example, like this:

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));
}

Probably, I suggested not an ideal variant, but in this code there is no implementation-defined behavior, and it will be easier for the reader to understand what is being tested.

You deserve a cup of coffee or tea. Take a break and continue: an interesting case of unspecified behavior awaits us.

At the interview, I ask the following as one of the first questions to the applicant: “What will the printf function print and why?”

int i = 5;
printf("%d,%d", i++, i++)

The correct answer is: unspecified behavior. The order of calculating the actual arguments when calling a function is not defined. Occasionally, I even demonstrate that this code compiled using Visual C ++ displays "6.5" on the screen, which is a complete dead end for newcomers who are weak in knowledge and spirit :).

It may seem like a contrived problem. But no, similar code can be found in serious applications, for example, in Android code.

bool ComposerClient::CommandReader::parseSetLayerCursorPosition(
  uint16_t length)
{
  if (length != CommandWriterBase::kSetLayerCursorPositionLength) {
    returnfalse;
  }
  auto err =
    mHal.setLayerCursorPosition(mDisplay, mLayer,
                                readSigned(), readSigned());
  if (err != Error::NONE) {
    mWriter.setError(getCommandLoc(), err);
  }
  returntrue;
}

Warning-Studio of PVS: V681 the 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 are interested in this line of code:

mHal.setLayerCursorPosition(...., readSigned(), readSigned());

By calling the readSigned function, two values ​​are read. But in what sequence the values ​​will be read, it is impossible to predict. This is a classic case of Unspecified Behavior.

The benefits of using a static code analyzer

This whole article popularizes static code analysis in general, and our PVS-Studio tool in particular. However, some errors are just perfect for demonstrating the power of static analysis. They are difficult to identify with code reviews, and only a non-tired program easily notices them. Consider a couple of such cases.

conststd::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:

• V766 CWE-462 An item with the same key "oem_lpass_cfg" has already been added. bootstat.cpp 264
• V766 CWE-462 An item with the same key "oem_xpu_ns_error" 'has already been added. bootstat.cpp 265

Different values ​​with identical keys are inserted into the sorted associative container ( std :: map ). In terms of the Common Weakness Enumeration, this is the CWE-462 : Duplicate Key in Associative List.

The text of the program is reduced, and the errors are marked with comments, so the error seems obvious, but when you just read this code with your eyes, it is very difficult to find such errors.

Consider another piece of code that is very difficult to read, because it is of the same type 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

If MTP_TYPE_UINT128 had to be caused by function putUInt128 , not putInt128 .

And the last in this section is a smart example of an unsuccessful Copy-Paste.

staticvoidbtif_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 reading the analyzer’s warning and the following text, I suggest that you look for the error yourself.

So that you do not accidentally read the answer, here is a picture for you to divert attention. If you are interested in what the egg with the java inscription means, go here .

So, I hope you enjoyed typing errors. Now is the time to bring a warning analyzer: V778 CWE-682 Two code fragments were found. Perhaps this is a typo and variable abort variable instead of 'continu'. btif_rc.cc 1554

Apparently, the code was written by the Copy-Paste method, and the person, as always, could not be attentive in the process of editing the copied code fragment. As a result, at the very end, he did not replace " continu " with " abort ".

Those. in the second block should be written:

avrc_rsp.abort.target_pdu = pavrc_cmd->abort.target_pdu;

This situation falls entirely within the definition of the “ last line effect ”, since an error occurred during the replacement of names at the end.

Facepalm

A very funny bug related to the conversion between little-endian and big-endian data formats (see Byte Order ).

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<constuint32_t*>(subsection_data);
  if (llvm::sys::IsLittleEndianHost !=
      m_Config.targets().isLittleEndian())
    bswap32(subsection_length);
  ....
}

PVS-Studio warning: V530 CWE-252 The return value of the 'bswap32' is required to be utilized. ELFAttribute.cpp 84

There are no complaints about the bswap32 function . But it is used incorrectly:

bswap32(subsection_length);

The author suggested that the variable is passed to the function by reference and changed there. However, you must use the return value of the function. As a result, no data conversion occurs.

The analyzer identified this error as CWE-252 : Unchecked Return Value. But, in fact, it is more appropriate to call here CWE-198 : Use of Incorrect Byte Ordering. Unfortunately, the analyzer cannot understand what the error is from a high-level point of view. However, this does not prevent him from identifying this serious defect in the code.

The correct code is:

subsection_length = bswap32(subsection_length);

There are three more places in the Android code with the same error:

• V530 CWE-252 The Return Value of Function 'bswap32' is required to be utilized. ELFAttribute.cpp 218
• V530 CWE-252 The Return Value of Function 'bswap32' is required to be utilized. ELFAttribute.cpp 346
• V530 CWE-252 The Return Value of Function 'bswap32' is required to be utilized. ELFAttribute.cpp 352

To avoid such errors, it is recommended to use the [[nodiscard]] attribute . This attribute is used to indicate that the return value of the function must be used when calling. Therefore, if you write this:

[[nodiscard]] inline uint32_t bswap32(uint32_t pData){ ... }

then the error would have been revealed at the stage of file compilation. You can learn more about some useful attributes from the article of my colleague " C ++ 17 ".

Unreachable code

In programming and compiler theory, unreachable code refers to a part of program code that cannot be executed under any circumstances, since it is unreachable in the control flow graph.

In terms of Common Weakness Enumeration, this is the 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 return statement must be explicitly lower.

Other errors of this type:

• V779 CWE-561 Unreachable code detected. It is possible that an error is present. bta_hf_client_main.cc 612
• V779 CWE-561 Unreachable code detected. It is possible that an error is present. android_media_ImageReader.cpp 468
• V779 CWE-561 Unreachable code detected. It is possible that an error is present. AMPEG4AudioAssembler.cpp 187

break

A forgotten break inside a switch is a classic mistake of C and C ++ programmers. To deal with it, in C ++ 17 there was such a useful abstract as [[fallthrough]] . You can read more about this error and [[fallthrough]] in my article " break and fallthrough ".

But while the world is full of old code, where [[fallthrough]] is not used, PVS-Studio will be useful to you. Consider 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 an error in the explanation does not need. I will only note that quite often an anomaly in the code is detected in more than one way. For example, this error is also detected by V519 warnings:

• V519 CWE-563 The 'codec_capability_.sample_rate' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 910, 916. a2dp_vendor_ldac.cc 916
• V519 CWE-563 The 'codec_config_.sample_rate' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 911, 917. a2dp_vendor_ldac.cc 917

And a couple more mistakes:

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

intReverb_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

Incorrect memory management

Here I have compiled errors related to incorrect memory management. Such warnings, according to Common Weakness Enumeration, are classified as:

• CWE-401 : Improve Release Memory Memory Reference ('Memory Leak')
• CWE-562 : Return of Stack Variable Address
• CWE-762 : Mismatched Memory Management Routines

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:

• V558 CWE-562 Function returns temporary reference object: tmp. transform_iterator.h 77
• V558 CWE-562 Function returns temporary reference object: tmp. transform_iterator.h 92

When functions finish their execution, the tmp variable is destroyed as it is created on the stack. Consequently, the functions return a reference to an already destroyed (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;
}

Consider even more sad code that deserves close attention.

intregister_socket_transport(
  int s, constchar* 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 Such a pointer will become invalid. transport.cpp 1030

This is a dangerous code. If the actual value of the serial argument is NULL, then a temporary buffer on the stack should be used. When the body of the if statement ends, the buf array will cease to exist. The place where the buffer was created can be used to store other variables created on the stack. A hellish mess in the data will begin, and the consequences of such an error are poorly predictable.

The following errors are related to incompatible ways to create and destroy objects.

void
SensorService::SensorEventConnection::reAllocateCacheLocked(....)
{
  sensors_event_t *eventCache_new;
  constint new_cache_size = computeMaxCacheSizeLocked();
  eventCache_new = newsensors_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 the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] mEventCache;'. Check lines: 391, 384. SensorEventConnection.cpp 391

Everything is simple here. A buffer, a pointer to which is stored in a member of the mEventCache class , is allocated using the new [] operator . And free this memory, using the operator delete . This is wrong and leads to undefined program behavior.

Similar error:

aaudio_result_t AAudioServiceEndpointCapture::open(....) {
  ....
  delete mDistributionBuffer;
  int distributionBufferSizeBytes =
    getStreamInternal()->getFramesPerBurst() *
    getStreamInternal()->getBytesPerFrame();
  mDistributionBuffer = newuint8_t[distributionBufferSizeBytes];
  ....
}

PVS-Studio warning: V611 CWE-762 The memory was allocated 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 the error is exactly the same.

structHeifFrameInfo
{
  ....
  voidset(....){
    ....
    mIccData.reset(newuint8_t[iccSize]);
    ....
  }
  ....
  std::unique_ptr<uint8_t> mIccData;
};

V554 CWE-762 Incorrect use of unique_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. HeifDecoderAPI.h 62

By default, the smart pointer class std :: unique_ptr calls the delete operator to destroy an object . However, in the set function, memory is allocated using the new [] operator .

The correct option is:

std::unique_ptr<uint8_t[]> mIccData;

Other errors:

• V554 CWE-762 Incorrect use of unique_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. atrace.cpp 949
• V554 CWE-762 Incorrect use of unique_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. atrace.cpp 950
• V554 CWE-762 Incorrect use of unique_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. HeifDecoderImpl.cpp 102
• V554 CWE-762 Incorrect use of unique_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. HeifDecoderImpl.cpp 166
• V554 CWE-762 Incorrect use of unique_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. ColorSpace.cpp 360

This section will complete the memory leak errors. An unpleasant surprise is that more than 20 such errors appeared. It seems to me that these can be very painful defects, leading to a gradual decrease in free memory during long-term continuous operation 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)
    returnNULL;
  pAsset->mAccessMode = mode;
  return pAsset;
}

PVS-Studio warning: V773 CWE-401 The pAsset pointer. A memory leak is possible. Asset.cpp 296

If a certain chunk could not be opened, the function ends its work without destroying the object, the pointer to which is stored in the variable pAsset . As a result, a memory leak will occur.

Other errors are similar, so I see no reason to consider them in the article. Those interested can see other warnings in the file: Android_V773.txt .

Overrun array

There are a large number of erroneous patterns that lead to the output of the array. In the case of Android, I found only one error pattern of the following type:

if (i < 0 || i > MAX)
  return;
A[i] = x;

In C and C ++, the cells of an array are numbered from 0, therefore the maximum index of an element in an array must be one less than the size of the array itself. The correct check should be:

if (i < 0 || i >= MAX)
  return;
A[i] = x;

Beyond the array boundary, according to the Common Weakness Enumeration, is classified as a CWE-119 : Improper Restriction of Memory Buffer.

Let's see how these errors look in the Android code.

staticbtif_hf_cb_t btif_hf_cb[BTA_AG_MAX_NUM_CLIENTS];
staticboolIsSlcConnected(RawAddress* bd_addr){
  if (!bd_addr) {
    LOG(WARNING) << __func__ << ": bd_addr is null";
    returnfalse;
  }
  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;
    returnfalse;
  }
  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 The

correct test option is:

if (idx < 0 || idx >= BTA_AG_MAX_NUM_CLIENTS) {

Exactly the same mistakes were found two more pieces:

• V557 CWE-119 Array overrun is possible. The value of 'idx' index could reach 6. btif_hf.cc 869
• V557 CWE-119 Array overrun is possible. The value of 'index' index could reach 6. btif_rc.cc 374

Broken loops

There are lots of ways to write a cycle that doesn't work properly. In the Android code, I encountered errors that, according to Common Weakness Enumeration, can be classified as:

• CWE-20 : Improper Input Validation
• CWE-670 : Always-Incorrect Control Flow Implementation
• CWE-691 : Insufficient Control Flow Management
• CWE-834 : Excessive Iteration

In this case, of course, there are other ways to "shoot yourself a leg" when writing cycles, but this time they did not meet me.

intmain(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 has detected that the constant EOF is compared with a variable of type char . Let's see why this code is incorrect.

The fgetc function returns a value of type int , namely: it can return a number from 0 to 255 or EOF (-1). The read value is placed in a variable of type char . Because of this, a character with a value of 0xFF (255) turns into -1 and is interpreted in the same way as the end of file (EOF).

Because of such errors, users using the Extended ASCII Codes sometimes face a situation where one of the characters in their alphabet is incorrectly processed by programs. For example, the last letter of the Russian alphabet in the Windows-1251 encoding just has the code 0xFF and is perceived by some programs as the end of the file.

Summarizing, we can say that the condition for stopping the cycle is written incorrectly. To remedy the situation, it is necessary that the variable c be of type int.

We continue and consider more familiar 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 the variable is being compared inside the 'for' operator. Consider reviewing 'i'. AudioPolicyManager.cpp 2489

Because of a typo in a nested loop, the condition uses the variable i , although you must use the variable j . As a result, the variable j is uncontrolledly incremented, which will eventually lead to the output of the mHwModules array . What happens next is impossible to predict, as there will be an undefined program behavior.

By the way, this fragment with an error was completely copied to another function. Therefore, the analyzer found the exact same error here: AudioPolicyManager.cpp 2586.

There are 3 more code fragments that are very suspicious to me. However, I do not presume to say that this code is exactly erroneous, since there is a complex logic there. In any case, I have to pay attention to this code for the author to check it.

The first fragment:

voidce_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. Check lines: 398, 452. ce_t3t.cc 452

Note that the variable i is used for both the outer and inner loop.

Two more similar analyzer triggers:

• V535 CWE-691 The variable 'xx' is being used for the loop. Check lines: 801, 807. sdp_db.cc 807
• V535 CWE-691 The variable 'xx' is being used for the loop. Check lines: 424, 438. nfa_hci_act.cc 438

You are not tired yet? I suggest to pause and download PVS-Studio in order to try to check your project.

Now let's continue.

#define NFA_HCI_LAST_PROP_GATE 0xFFtNFA_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

Note the following:

• The constant NFA_HCI_LAST_PROP_GATE is equal to 0xFF.
• A variable of the uint8_t type is used as a loop counter . Therefore, the range of values ​​of this variable is [0..0xFF].

It turns out that the condition gate_id <= NFA_HCI_LAST_PROP_GATE is always true and cannot stop the execution of the loop.

The analyzer has classified this error as CWE-834, but it can also be interpreted as CWE-571: Expression is Always True.

The next error in the cycle is associated with 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 accept all possible values ​​for the variable int and only then the cycle ended.

The loop should stop when the expression ++ retries will be equal to 0. And this is possible only if there is an overflow of the variable. Since a variable has a sign type, its overflow leads to undefined behavior. Therefore, this code is incorrect and can lead to unpredictable consequences. For example, the compiler has the full right to remove the check and leave only the instruction for the increment of the counter.

And the last mistake in this section.

status_t Check(conststd::string& source) {
  ....
  int pass = 1;
  ....
  do {
    ....
    switch(rc) {
    case0:
      SLOGI("Filesystem check completed OK");
      return0;
    case2:
      SLOGE("Filesystem check failed (not a FAT filesystem)");
      errno = ENODATA;
      return-1;
    case4:
      if (pass++ <= 3) {
          SLOGW("Filesystem modified - rechecking (pass %d)",
                  pass);
          continue;                                         // <=
      }
      SLOGE("Failing check after too many rechecks");
      errno = EIO;
      return-1;
    case8:
      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);                                              // <=return0;
}

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

Before us is a loop of the form:

do {
  ....
  if (x) continue;
  ....
} while (0)

To perform repeated iterations, the programmer uses the continue statement . It is not right. The continue statement does not resume the loop immediately, but proceeds to check the condition. Since the condition is always false, the cycle in any case will be executed only once.

To correct the error, the code can be rewritten, for example, like this:

for (;;) {
  ....
  if (x) continue;
  ....
  break;
}

Reassigning a variable

A very common mistake is to re-write to a variable even before the previous value was used. Most often, such errors occur due to a typo or unsuccessful copy-paste. According to the Common Weakness Enumeration, such errors are classified as CWE-563 : Assignment to Variable without Use. Not without such errors in Android.

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

To isolate the essence of the error, I will write pseudo-code:

if (a > 0)
  X = 1;
else
  X = 2;
X = 1;

Regardless of the condition, the variable X (in the present code, this is namespaceExt.prefix.index ) will always be assigned one 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 is not clear why it was necessary to initialize the variable when declaring, if then another value is immediately written to it. 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 suggest what should have been written:

last_visited_internal_latency_ += kArmMemoryLoadLatency;

And the last mistake, demonstrating how the analyzer tirelessly finds errors that are likely to be missed even with a careful code review.

voidmultiprecision_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 “ tear out the eyes” that I do not want to understand it. At the same time, it is clearly visible: here there is a typo in the code, which I highlighted with comments.

Other errors

There were isolated errors, for which there is no point in making separate chapters. However, they are as interesting and insidious as previously discussed.

Priority of operations

void TagMonitor::parseTagsToMonitor(String8 tagNames) {
  std::lock_guard<std::mutex> lock(mMonitorMutex);
  // Expand shorthandsif (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 the following: 'A = (B! = C)'. TagMonitor.cpp 50

According to the Common Weakness Enumeration classification: CWE-783 : Operator Precedence Logic Error.

The programmer thought of the following. Substring “3a” is searched for and the idx position is written to the idx variable . If the substring is found (idx! = -1), then the code in which the value of the idx variable is used is executed .

Unfortunately, the programmer is confused about the priorities of operations. In fact, the test works like this:

if (ssize_t idx = (tagNames.find("3a") != -1))

First, it is checked whether there is a substring “3a” in the string, and the result false / true is placed in the variable idx . As a result, the idx variable has the value 0 or 1.

If the condition is true (the idx variable is equal to 1), then the logic using the idx variable will be executed . A variable always equal to 1 will lead to incorrect program behavior.

You can correct the error if you take out the initialization of the variable 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

structHearingDevice {
  ....
  HearingDevice() { HearingDevice(RawAddress::kEmpty, false); }
  ....
};

PVS-Studio Warning: V603 CWE-665 If you wish to call the constructor, 'this-> HearingDevice :: HearingDevice (....)' should be used. hearing_aid.cc 176

According to the Common Weakness Enumeration classification: CWE-665 : Improper Initialization.

Programmers are often mistaken in trying to explicitly call a constructor to initialize an object. There are two constructors in the class. To reduce the size of the source code, the programmer decided to call one constructor from another. But this code does not exactly what the developer expects.

The following happens. A new unnamed object of type HearingDevice is created and immediately destroyed. As a result, the class fields remain uninitialized.

To correct the error, you can use the delegating constructor (this feature appeared in C ++ 11). The correct code is:

HearingDevice() : HearingDevice(RawAddress::kEmpty, false) { }

The function does not return a value.

intNET_RecvFrom(int s, void *buf, int len, unsignedint 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 Common Weakness Enumeration classification: CWE-393 : Return of Wrong Status Code.

The function will return a random value. Another such error: V591 CWE-393 Non-void function should return a value. linux_close.cpp 158

Incorrect structure size calculation

int MtpFfsHandle::handleControlRequest(....) {
  ....
  structmtp_device_status *st =
    reinterpret_cast<struct mtp_device_status*>(buf.data());
  st->wLength = htole16(sizeof(st));
  ....
}

PVS-Studio Warning: V568 It’s an odd one that the size of the (st) class object. MtpFfsHandle.cpp 251

I am sure that the wLength member variable wanted to put the size of the structure, not the size of the pointer. Then the correct code should be:

st->wLength = htole16(sizeof(*st));

Similar analyzer triggers:

• If you’re a sizeof () ’’ NetlinkEvent.cpp 220
• If you’re a sizeof () ’ linker_block_allocator.cpp 146
• V568 It's odd that the operator of the sizeof () operator is the '& session_id' expression. reference-ril.c 1775

Meaningless 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 0x00000004EGLContext 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 Common Weakness Enumeration classification: CWE-480 : Use of Incorrect Operator.

Expression of the form (A | 1) || (A | 2) || (A | 4) does not make sense, since the result will always be true. In fact, the & operator must be used , and then the code takes on the meaning:

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))

Similar error: V617 CWE-480 Consider inspecting the condition. The '0x00000001' argument of the '|' bitwise operation contains a non-zero value. egl.cpp 1338

Wrong bit shift

template <typename AddressType>
structRegsInfo {
  ....
  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 type. RegsInfo.h 47

According to the Common Weakness Enumeration classification: CWE-190 : Integer Overflow or Wraparound.

With a shift of 1 << reg, the value of the variable reg lies in the range [0..63]. The expression serves to obtain different powers of two, starting with 2 ^ 0 and ending with 2 ^ 63.

The code does not work. The fact is that the numeric literal 1 has a 32-bit type int . Therefore, it will not be possible to get a value greater than 1 ^ 31. A shift to a larger value causes a variable to overflow and an undefined behavior.

The correct code is:

saved_reg_map |= static_cast<uint64_t>(1) << reg;

or:

saved_reg_map |= 1ULL << reg;

The lines are copied to themselves.

void PCLmGenerator::writeJobTicket() {
 // Write JobTicketchar 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:

• V549 CWE-688 The first argument of the 'strcpy' function is equal to the second argument. genPCLm.cpp 1181
• V549 CWE-688 The first argument of the 'strcpy' function is equal to the second argument. genPCLm.cpp 1182

According to the Common Weakness Enumeration classification: CWE-688 : Function Call With Incorrect Variable or Reference as Argument.

Lines are for some reason copied into themselves. Most likely, there are some typos.

Using an uninitialized variable

voidmca_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 the memcpy function. mca_main.cc 252

According to the Common Weakness Enumeration classification: CWE-824 : Access of Uninitialized Pointer.

If p_tbl-> tcid! = MCA_CTRL_TCID and p_dcb == nullptr , then the p_opt pointer will remain uninitialized.

Stranger use of uninitialized variable

structtimespec
{__time_t tv_sec;    /* Seconds.  */longint tv_nsec;   /* Nanoseconds.  */
};
staticinline 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 Common Weakness Enumeration classification: CWE-457 : Use of Uninitialized Variable.

At the moment of decrement of the t.tv_nsec variable , it is uninitialized. The variable is initialized later: t.tv_nsec = remainder; . Something is clearly messed up here.

Redundant expression

voidbta_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

This expression is redundant. If you remove the subexpression (bte_appl_cfg.ble_auth_req & 0x04) , the result of the expression will not change. Perhaps there is some typo here.

Handle leak

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());
    returnfalse;
  }
  unsignedchar 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";
  returntrue;
}

PVS-Studio warning: V773 CWE-401 The pfin 'handle. A resource leak is possible. slang_rs_reflection_cpp.cpp 448

The analyzer has classified this error, according to the Common Weakness Enumeration, as: CWE-401 : Improved Release Memory Memory Card Reminder Last Reference ('Memory Leak'). However, it would be more appropriate to issue here a CWE-775 : Missing Release of the Descriptor or Handle after Effective Lifetime. I will instruct my colleagues to correct this flaw in PVS-Studio.

The pfin handle is not released anywhere. Just forgot to call fclose at the end.. An unpleasant error that can quickly exhaust the entire stock of available descriptors, after which it will be impossible to open new files.

Conclusion

As you can see, even in a well-known and well-tested project like Android, the PVS-Studio analyzer easily finds many errors and potential vulnerabilities. To summarize, what weaknesses (potential vulnerabilities) were found:

• CWE-14: Compiler Removal of Code to Clear Buffers
• CWE-20: Improper Input Validation
• CWE-119: Improve Restriction of Memory Buffer
• CWE-190: Integer Overflow or Wraparound
• CWE-198: Use of Incorrect Byte Ordering
• CWE-393: Return of Wrong Status Code
• CWE-401: Improve Release Memory Memory Reference ('Memory Leak')
• CWE-457: Use of Uninitialized Variable
• CWE-462: Duplicate Key in Associative List
• CWE-480: Use of Incorrect Operator
• CWE-484: Omitted Break Statement in Switch
• CWE-561: Dead Code
• CWE-562: Return of Stack Variable Address
• CWE-563: Assignment to Variable without Use
• CWE-570: Expression is Always False
• CWE-571: Expression is Always True
• CWE-476: NULL Pointer Dereference
• CWE-628: Function Call with Incorrectly Specified Arguments
• CWE-665: Improper Initialization
• CWE-670: Always-Incorrect Control Flow Implementation
• CWE-682: Incorrect Calculation
• CWE-688: Function Call With Incorrect Variable or Reference as Argument
• CWE-690: Unchecked Return Value to NULL Pointer Dereference
• CWE-691: Insufficient Control Flow Management
• CWE-758: Reliance on Undefined, Unspecified, or Implementation-Defined Behavior
• CWE-762: Mismatched Memory Management Routines
• CWE-775: Missing Release of File Descriptor or Handle after Effective Lifetime
• CWE-783: Operator Precedence Logic Error
• CWE-824: Access of Uninitialized Pointer
• CWE-834: Excessive Iteration

In total, I gave a description of 490 potential vulnerabilities in the article. In fact, the analyzer is able to identify them more, but, as I wrote earlier, I could not find the strength to study the report more carefully.

The size of the proven code base is about 2168000 lines of code in C and C ++. Of these, 14.4% are comments. Total, we receive about 1855000 lines of the pure code.

Thus, we have 490 CWE for 1,855,000 lines of code.

It turns out that the PVS-Studio analyzer can detect more than 1 weakness (potential vulnerability) for every 4000 lines of code in the Android project. Good result for the code analyzer, I'm happy.

Thanks for attention! I wish you all a hopeless code, and I propose to do the following:

1. Download PVS-Studio and check the working draft.
2. Just ask: do not run the analyzer on synthetic tests: Why I do not like synthetic tests .
3. Subscribe to be aware of the publication of our new articles: twitter , RSS , vk.com .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. We checked the Android Source Codes by PVS-Studio or Nothing is Perfect