[SERVER-28570] Analysis of 'MongoDB' source code by PVS-Studio Created: 31/Mar/17  Updated: 31/May/17  Resolved: 02/May/17

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Sergey [X] Assignee: Kelsey Schubert
Resolution: Duplicate Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
duplicates SERVER-29002 Fix range check in IndexBoundsBuilder... Closed
duplicates SERVER-29003 Expression 'i >= 0' is always true. U... Closed
Participants:

 Description   

Hello,
To demonstrate the abilities of PVS-Studio analyzer, we decided to find several bugs.

Status verifySystemIndexes(OperationContext* opCtx) {
  ....
  IndexCatalog* indexCatalog = collection->getIndexCatalog();
  std::vector<IndexDescriptor*> indexes;
  indexCatalog->findIndexesByKeyPattern(opCtx,                    // <=
                                        v1SystemUsersKeyPattern, 
                                        false, 
                                        &indexes);
  if (indexCatalog && !indexes.empty()) {                         // <=
    ....
  }
  ....
}

A link to the source code on GitHub

PVS-Studio warning: V595 The 'indexCatalog' pointer was utilized before it was verified against nullptr. Check lines: 100, 102. auth_index_d.cpp 100

In the conditional expression of the if statement there is a check that the pointer indexCatalog is non-null. But earlier in the code we see a call of the method findIndexesByKeyPattern via the pointer indexCatalog. Thus, if indexCatalog is a null pointer, then in the line 100 we'll have a null pointer dereference before the necessary check. If the pointer indexCatalog can never be null, then the statement indexCatalog && !indexes.empty() is redundant.

string IndexBoundsBuilder::simpleRegex(....) {
  ....
  if (c == 'Q') {
    ....
  } else if ((c >= 'A' && c <= 'Z') || 
             (c >= 'a' && c <= 'z') || 
             (c >= '0' && c <= '0') ||          // <=
             (c == '\0')) {
    ....
  }
  ....
}

A link to the source code on GitHub

PVS-Studio warning: V590 Consider inspecting the 'c >= '0' && c <= '0'' expression. The expression is excessive or contains a misprint. index_bounds_builder.cpp 145

Most likely, the subexpression c >= '0' && c <= '0' has an error, there is no range check of the symbol (this subexpression will be true only in case c == '0'). Judging by other subexpressions, supposedly it should be as follows: c >= '0' && c <= '9'.

void* MemoryMappedFile::map(....) {
  ....
  size_t len = strlen(filename);
  for (size_t i = len - 1; i >= 0; i--) {            // <=
    if (filename[i] == '/' || filename[i] == '\\')
      break;
 
    if (filename[i] == ':')
      filename[i] = '_';
  }
  ....
}

A link to the source code on GitHub

PVS-Studio warning: V547 Expression 'i >= 0' is always true. Unsigned type value is always >= 0. mmap_windows.cpp 197

The conditional statement of the loop (i >= 0) should always be true, as the loop counter ( i ) is of unsigned type (size_t).

PVS-Studio is a tool for bug detection in the source code of programs, written in C, C++ and C#. It works in Windows and Linux environment. https://www.viva64.com/en/pvs-studio/
We suggests having a look at the emails, sent from @viva64.com.

Best regards,
Sergey Vasiliev



 Comments   
Comment by Kelsey Schubert [ 02/May/17 ]

Hi Night walker,

Thank you for the bug reports. I've created SERVER-29002 and SERVER-29003 to track the work to correct these issues. Please feel free to watch these tickets for updates.

Kind regards,
Thomas

Comment by Sergey [X] [ 31/Mar/17 ]

I apologize that I've written an incorrect e-mail address, the e-mails were sent from @pvs-studio.com, not from @viva64.com.

Generated at Thu Feb 08 04:18:30 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.