opensource

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.

Ghidra

/ hownot2code / Leave a comment

Ghidra

Always true & Unreachable code

public void setValueAt(Object aValue, int row, int column) {
  ...
  int index = indexOf(newName);
  if (index >= 0) {                  // <=
    Window window = tool.getActiveWindow();
    Msg.showInfo(getClass(), window, "Duplicate Name",
                 "Name already exists: " + newName);
    return;
  }

  ExternalPath path = paths.get(row); // <=
  ...
}
private int indexOf(String name) {
  for (int i = 0; i < paths.size(); i++) {
    ExternalPath path = paths.get(i);
    if (path.getName().equals(name)) {
      return i;
    }
  }
  return 0;
}

PVS-Studio warnings:

  • V6007 Expression ‘index >= 0’ is always true. ExternalNamesTableModel.java:105
  • V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

Something distracted the developer, and they accidentally implemented the indexOf method in such a way that it returns 0, i.e. the index of the first element of the paths collection, instead of -1 for a non-existent value. This will happen even if the collection is empty. Or maybe they generated the method but forgot to change the default return value. Anyway, the setValueAt method will refuse any offered value and show the message “Name already exists” even if there’s not a single name in the collection.

By the way, the indexOf method is not used anywhere else, and its value is actually needed only to determine if the sought element exists. Rather than writing a separate method and playing around with indexes, it would probably be better to write a for-each loop right in the setValueAt method and have it return when encountering the matching element.

Please click here to see more bugs from this project.

RavenDB

/ hownot2code / Leave a comment

RavenDB JPG_Logo-2

Always true

public override void VerifyCanExecuteCommand(
  ServerStore store, TransactionOperationContext context, bool isClusterAdmin
)
{
  using (context.OpenReadTransaction())
  {
    var read = store.Cluster.GetCertificateByThumbprint(context, Name);
    if (read == null)
      return;

    var definition = JsonDeserializationServer.CertificateDefinition(read);
    if (
      definition.SecurityClearance != SecurityClearance.ClusterAdmin || // <=
      definition.SecurityClearance != SecurityClearance.ClusterNode     // <=
    )
      return;
  }

  AssertClusterAdmin(isClusterAdmin);
}

Analyzer warning: V3022 Expression is always true. Probably the ‘&&’ operator should be used here. DeleteCertificateFromClusterCommand.cs(21) Raven.Server

Another example of a situation where almost certainly the wrong logical operator was chosen. In this case, the condition is always true, because the variable isn’t exactly equal to at least one of the values that it is compared with.

I suppose that “||” should be replaced with “&&”. Then the above fragment will make sense. If the logical operator is chosen correctly, it is most likely that other variables should be compared in one of the conditions. Anyway, this fragment looks very fishily and it must be analyzed.

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.

How to shoot yourself in the foot in C and C++. Haiku OS Cookbook

/ hownot2code / Leave a comment

This story goes back to 2015, when Haiku OS and PVS-Studio static analyzer developers decided to join forces and improve this OS code quality. At first it was more like an experiment, as there was no Linux analyzer at that time and the team had to work only with the compiled executable analyzer file. The entire infrastructure for parsing compiler parameters, running preprocessor, analysis paralleling and so on was taken from the Compiler Monitoring UI utility in C#, which was ported in parts to the Mono platform in order to be run in Linux.

Artboard 1 copy (1)
Continue reading →