Command & Conquer

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

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

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

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

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

Android Operating System: One Potential Vulnerability per 4000 Lines of C++ Code

 

For many years, Andrey Karpov has been publishing articles on code quality, and bugs reviews of open source projects. For example, he is the author of such publications as “The Ultimate Question of Programming, Refactoring, and Everything” and “27 000 Errors in the Tizen Operating System“.

image1

Recently, the open source Android operating system has become of interest for him. He researched that part of the operating system code, which is written in the C and C++ languages. After that he came to a conclusion, which always takes place after such research: human error is always possible. By using the PVS-Studio tool, it becomes possible to detect at least one security defect (potential vulnerability) per 4000 lines of code.

Continue reading