C/C++ bugs of the month

QEMU

/ hownot2code / Leave a comment

Array overrun

V557 Array overrun is possible. The ‘dwc2_glbreg_read’ function processes value ‘[0..63]’. Inspect the third argument. Check lines: 667, 1040. hcd-dwc2.c 667

Warning N4V557 Array overrun is possible. The 'dwc2_glbreg_read' function processes value '[0..63]'. Inspect the third argument. Check lines: 667, 1040. hcd-dwc2.c 667#define HSOTG_REG(x) (x)                                             // <= 5
....
struct DWC2State {
  ....
#define DWC2_GLBREG_SIZE    0x70
  uint32_t glbreg[DWC2_GLBREG_SIZE / sizeof(uint32_t)];              // <= 1
  ....
}
....
static uint64_t dwc2_glbreg_read(void *ptr, hwaddr addr, int index,
                                 unsigned size)
{
  ....
  val = s->glbreg[index];                                            // <= 2
  ....
}
static uint64_t dwc2_hsotg_read(void *ptr, hwaddr addr, unsigned size)
{
  ....
  switch (addr) {
    case HSOTG_REG(0x000) ... HSOTG_REG(0x0fc):                      // <= 4
        val = dwc2_glbreg_read(ptr, addr,
                              (addr - HSOTG_REG(0x000)) >> 2, size); // <= 3
    ....
  }
  ....
}

This code has a potential problem – an index outside the array bounds. The DWC2State structure defines a glbreg array consisting of 28 elements (comment 1). In the dwc2_glbreg_read function, our array is accessed by index (comment 2). Now note that the function dwc2_glbreg_read is passed the expression (addr – HSOTG_REG(0x000)) >> 2 (comment 3) as an index, which can take a value in the range [0..63]. To make sure of it, pay attention to comments 4 and 5. Perhaps, the range of values from comment 4 has to be fixed. 

Please click here to see more bugs from this project.

Command & Conquer

/ hownot2code / Leave a comment

270599

Suspicious formatting

V705 It is possible that ‘else’ block was forgotten or commented out, thus altering the program’s operation logics. NETDLG.CPP 1506

static int Net_Join_Dialog(void)
{
  ....
  /*...............................................................
  F4/SEND/'M' = edit a message
  ...............................................................*/
  if (Messages.Get_Edit_Buf()==NULL) {
    ....
  } else  /*...............................................................
  If we're already editing a message and the user clicks on
  'Send', translate our input to a Return so Messages.Input() will
  work properly.
  ...............................................................*/
  if (input==(BUTTON_SEND | KN_BUTTON)) {
    input = KN_RETURN;
  }
  ....
}

Due to a large comment, the developer hasn’t seen the above unfinished conditional operator. The remaining else keyword forms the else if construction with the condition below, which most likely changes the original logic.

Please click here to see more bugs from this project.

Command & Conquer

/ hownot2code / Leave a comment

270599

Array overrun

V557 Array overrun is possible. The ‘QuantityB’ function processes value ‘[0..86]’. Inspect the first argument. Check lines: ‘HOUSE.H:928’, ‘CELL.CPP:2337’. HOUSE.H 928

typedef enum StructType : char {
  STRUCT_NONE=-1,
  ....
  STRUCT_COUNT,   // <= 87
  STRUCT_FIRST=0
} StructType;
int BQuantity[STRUCT_COUNT-3];
int QuantityB(int index) {return(BQuantity[index]);}
bool CellClass::Goodie_Check(FootClass * object)
{
  ....
  int bcount = 0;
  for( j=0; j < STRUCT_COUNT; j++) {     bcount += hptr->QuantityB(j);
  }
  ....
}

There are a lot of global variables in the code and it is obvious that they are easy to get confused. The analyzer’s warning about an array index out of bounds is issued at the point of accessing the BQuantity array by index. The array size is 84 elements. Algorithms for analyzing the data flow in the analyzer helped to find out that the index value comes from another function – Goodie_Check. There, a loop is executed with a final value of 86. Therefore, 12 bytes of “someone’s” memory (3 int elements) are constantly being read in this place.

Please click here to see more bugs from this project.

Kodi

/ hownot2code / Leave a comment

Kodi

Missed semicolon

PVS-Studio warning: V504 It is highly probable that the semicolon ‘;’ is missing after ‘return’ keyword. AdvancedSettings.cpp:1476

void CAdvancedSettings::SetExtraArtwork(const TiXmlElement* arttypes,
   std::vector& artworkMap)
{
  if (!arttypes)
    return
  artworkMap.clear();
  const TiXmlNode* arttype = arttypes->FirstChild("arttype");
  ....
}

The code formatting suggests the following execution logic:

  • if arttypes is a null pointer, the method returns;
  • if arttypes is a non-null pointer, the artworkMap vector gets cleared and some actions are then performed.

But the missing ‘;’ character breaks it all, and the actual execution logic is as follows:

  • if arttypes is a null pointer, the artworkMap vector gets cleared and the method returns;
  • if arttypes is a non-null pointer, the program executes whatever actions come next but the artworkMap vector doesn’t get cleared.

To cut a long story short, this situation does look like a bug. After all, you hardly expect anyone to write expressions like return artworkMap.clear(); :).

Please click here to see more bugs from this project.