📕 😎 🤚 NSA，Ghidra和独角兽 🔏 🏦 👩🏾‍⚖️

这次，PVS-Studio团队被Ghidra所吸引，Ghidra是一个用于逆向工程的庞大而邪恶的框架，您可以使用该框架分析各种二进制文件并使用它们进行各种可怕的事情。最有趣的不是它可以免费使用或很好地扩展插件，而是由NSA编写并发布在GitHub上的。一方面，NSA似乎有足够的资源来保持代码库的整洁。另一方面，对它不太熟悉的新贡献者最近可能会意外添加未发现的错误。因此，在进行了静态分析之后，我们决定寻找此项目中的弱点。

序幕

总的来说，PVS-Studio静态分析器在Ghidra项目的Java部分（版本9.1.2，提交687ce7f）中发出651高，904中和909低警告。其中，大约一半的高中响应是由V6022诊断程序触发的。在“方法主体”中不使用参数，该参数通常在重构后出现，当不再需要某些参数或某些功能被注释暂时禁用时。快速查看一下这些警告（它们太多了，无法作为外部观察者来查看每个警告））在此项目中没有发现任何明显可疑的东西。可能允许该项目在分析器设置中暂时禁用此诊断以免被它分散注意力。实际上，一个人经常会在setter或构造函数参数的名称中遇到错别字，通常来说，它不应该我敢肯定，大多数读者至少会遇到一次类似的不愉快模式：

public class A {
  private String value;
  public A(String val) { // V6022
    this.value = value;
  }
  public int hashCode() {
    return value.hashCode(); // NullPointerException
  }
}

低警告的一半以上是由“变量的V6008潜在的空取消引用” 诊断产生的-例如，通常使用值File.getParentFile（）而不检查null。如果在没有绝对路径的情况下构造了调用此方法的文件对象，则将返回null，并且缺少验证可能会丢弃该应用程序。

按照传统，我们将仅分析高中级别的警告，因为其中包含了大量实际错误。使用分析器报告时，我们始终建议您按可靠性的降序对警告进行分析。

接下来，我们考虑分析仪指示的一些片段，这些片段对我来说似乎可疑或有趣。事实证明，该项目的代码库规模巨大，几乎不可能手动找到这些地方。

片段1：验证无效

private boolean parseDataTypeTextEntry()
throws InvalidDataTypeException {
  ...
  try {
    newDataType = parser.parse(selectionField.getText(),
                               getDataTypeRootForCurrentText());
  }
  catch (CancelledException e) {
    return false;
  }
  if (newDataType != null) {
    if (maxSize >= 0
        && newDataType.getLength() > newDataType.getLength()) { // <=
      throw new InvalidDataTypeException("data-type larger than "
                                         + maxSize + " bytes");
    }
    selectionField.setSelectedValue(newDataType);
    return true;
  }
  return false;
}

PVS-Studio警告：V6001在'>'运算符的左侧和右侧有相同的子表达式'newDataType.getLength（）'。 DataTypeSelectionEditor.java data66

此类提供用于选择支持自动完成的数据类型的图形组件。使用此组件的开发人员可以设置所选数据类型的最大允许大小（通过maxSize字段），也可以通过设置负值使其不受限制。假定在验证输入的数据时，超出限制将引发异常，然后将异常捕获到调用堆栈中，并向用户显示一条消息。

似乎该组件的作者在编写此测试时就分心了，或者也许他在思考生命的意义，但是最后，根本就没有进行验证，因为该数字永远不能大于其本身，因此我们忽略了这种情况。这意味着该组件可以提供无效数据。

在另外两个类中发现了另一个类似的错误：GuidUtil和NewGuid。

public class GuidUtil {
  ...
  public static GuidInfo parseLine(...) {
    ...
    long[] data = new long[4];
    ...
    if (isOK(data)) {
      if (!hasVersion) {
        return new GuidInfo(guidString, name, guidType);
      }
      return new VersionedGuidInfo(guidString, version, name, guidType);
    }
    return null;
  }
  ...
  private static boolean isOK(long[] data) {
    for (int i = 0; i < data.length; i++) {
      if ((data[i] != 0) || (data[i] != 0xFFFFFFFFL)) { // <=
        return true;
      }
    }
    return false;
  }
  ...
}

PVS-Studio 警告：V6007表达式'data [i]！= 0xFFFFFFFFL'始终为true。 GuidUtil.java:200

的对的循环的ISOK方法检查该相同值不同时等于两个不同的数字。如果是这样，则GUID立即被识别为有效。也就是说，仅当数据数组为空时，GUID才是无效的，并且永远不会发生，因为在parseLine方法的开始处，仅对一次分配了相应变量的值。IsOK

方法主体在这两个类中它完全重合，这表明了另一种错误代码的复制粘贴的想法。我不确定作者到底想检查什么，但是我可以假定应按以下方式修复此方法：

private static boolean isOK(long[] data) {
  for (int i = 0; i < data.length; i++) {
    if ((data[i] == 0) || (data[i] == 0xFFFFFFFFL)) {
      return false;
    }
  }
  return true;
}

片段2：隐藏异常

public void putByte(long offsetInMemBlock, byte b)
throws MemoryAccessException, IOException {
  long offsetInSubBlock = offsetInMemBlock - subBlockOffset;
  try {
    if (ioPending) {
      new MemoryAccessException("Cyclic Access"); // <=
    }
    ioPending = true;
    doPutByte(mappedAddress.addNoWrap(offsetInSubBlock / 8),
              (int) (offsetInSubBlock % 8), b);
  }
  catch (AddressOverflowException e) {
    new MemoryAccessException("No memory at address"); // <=
  }
  finally {
    ioPending = false;
  }
}

PVS-Studio警告：V6006已创建对象，但未使用该对象。可能没有'throw'关键字：'new MemoryAccessException（“ Cyclic Access”）'。 BitMappedSubMemoryBlock.java:99

如您所知，异常对象本身不执行任何操作（或至少不应执行任何操作）。几乎总是，它们的新实例是通过throw抛出的，在极少数情况下-转移到某个地方或放在集合中。

包含此方法的类是对存储块的包装，该存储块允许读取和写入数据。在这里，由于没有引发异常，因此可能会违反使用ioPending标志对当前内存块进行访问的限制另外，还将忽略AddressOverflowException。因此，数据可以被静默破坏，并且开发人员将收到必须由调试器进行分析的奇怪工件，而不是明确指示特定位置的错误。

这些丢失的异常中有八种：

BitMappedSubMemoryBlock.java：第77、99、106、122行
ByteMappedSubMemoryBlock.java：第52、73、92、114行

特征是在同一文件中存在极其相似的抛出方法。最有可能的是，一种方法最初是与上述片段类似地编写的，之后被复制了几次，以某种方式发现了错误，并在他们可以记住的地方纠正了该错误。

片段3：雷区

private void processSelection(OptionsTreeNode selectedNode) {
  if (selectedNode == null) {
    setViewPanel(defaultPanel, selectedNode); // <=
    return;
  }
  ...
}
private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {
  ...
  setHelpLocation(component, selectedNode);
  ...
}
private void setHelpLocation(JComponent component, OptionsTreeNode node) {
  Options options = node.getOptions();
  ...
}

PVS-Studio 警告：V6008函数'setViewPanel'中'selectedNode'的空取消引用。 OptionsPanel.java:266

分析器撒了谎-目前，调用processSelection方法不会导致NullPointerException，因为此方法仅被调用了两次，并且在调用之前，显式检查selectedNode是否为null。不应这样做，因为另一个开发人员可以看到该方法显式处理了selectedNode == null的情况，并确定这是一个有效值，这将导致应用程序崩溃。这样的惊喜在开放项目中尤其危险，因为不了解代码库的人会完全参与其中。

通常，我必须说整个processSelection方法看起来很奇怪。这可能是一个复制粘贴错误，因为在相同的方法中，尽管条件不同，但具有相同主体的if块会再遇到两次。但是，此时，selectedNode不再为null，并且setViewPanel-setHelpLocation调用链不会导致NullPointerException。

片段4：自动完成以作恶用

public static final int[] UNSUPPORTED_OPCODES_LIST = { ... };
public static final Set<Integer> UNSUPPORTED_OPCODES = new HashSet<>();

static {
  for (int opcode : UNSUPPORTED_OPCODES) {
    UNSUPPORTED_OPCODES.add(opcode);
  }
}

PVS-Studio警告：V6053正在进行迭代时，将修改集合。 ConcurrentModificationException可能会发生。 DWARFExpressionOpCodes.java:205

在这种情况下，分析器再次撒了谎-不会引发异常，因为UNSUPPORTED_OPCODES集合始终为空，并且循环根本不会执行。另外，碰巧集合是多种多样的，添加一个已经存在的元素不会改变它。作者很可能输入for-each通过自动完成收集的名称，并且没有注意到提出了错误的字段。不可能在迭代过程中修改集合，但是在良好的情况下（如在这种情况下），应用程序可能不会崩溃。在这里，这种错字有间接的影响：解析DWARF文件的机器在发现不受支持的操作码时将依赖此集合来停止分析。

从Java 9开始，值得将标准库的工厂方法用于常量集合：例如，Set.of（T ... elements）不仅更加方便，而且可以立即使创建的集合不可变，从而提高了代码的可靠性。

片段5：应有尽有

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警告：

V6007表达式'index> = 0'始终为true。ExternalNamesTableModel.java:105
V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

作者对此进行了考虑，并且在indexOf方法中，对于未知值，不是“ index” -1而是返回0- paths集合的第一个元素的索引。即使集合为空。或者可能是生成了方法，但是忘记更改默认返回值。结果，setValueAt方法将丢弃传递给它的任何值，并向用户显示“名称已存在”错误，即使没有名称也是如此。

顺便说一句，indexOf不会在其他任何地方使用，仅在确定您要查找的元素是否存在时才需要它的值。也许，而不是单独的方法，直接在setValueAt中编写for-each并返回在匹配项上，而不是带有索引的游戏上。

注意：我无法重现所谓的错误。仅在某些情况下，可能不再使用或调用setValueAt方法。

片段6：保持沉默

final static Map<Character, String> DELIMITER_NAME_MAP = new HashMap<>(20);
// Any non-alphanumeric char can be used as a delimiter.
static {
  DELIMITER_NAME_MAP.put(' ', "Space");
  DELIMITER_NAME_MAP.put('~', "Tilde");
  DELIMITER_NAME_MAP.put('`', "Back quote");
  DELIMITER_NAME_MAP.put('@', "Exclamation point");
  DELIMITER_NAME_MAP.put('@', "At sign");
  DELIMITER_NAME_MAP.put('#', "Pound sign");
  DELIMITER_NAME_MAP.put('$', "Dollar sign");
  DELIMITER_NAME_MAP.put('%', "Percent sign");
  ...
}

PVS-Studio警告：V6033已经添加了具有相同键“ @”的项目。 FilterOptions.java:45

Ghidra支持在各种上下文中过滤数据：例如，您可以按名称过滤项目文件列表。另外，同时实现了通过几个关键字的过滤：'.java，.c'处于'OR'模式时，显示名称包含'.java'或'.c'的所有文件。可以理解，任何特殊字符都可以用作单词分隔符（在过滤器设置中选择了特定的分隔符），但实际上没有感叹号。

在此类初始化表中，密封起来非常容易，因为它们通常是使用复制粘贴编写的，并且当您查看此类代码时，您的眼睛很快就会模糊。如果错别字不在两条相邻的线上，那么用手肯定几乎没人会看到。

片段7：除法余数始终为0

void setFactorys(FieldFactory[] fieldFactorys,
                 DataFormatModel dataModel, int margin) {
  factorys = new FieldFactory[fieldFactorys.length];

  int x = margin;
  int defaultGroupSizeSpace = 1;
  for (int i = 0; i < factorys.length; i++) {
    factorys[i] = fieldFactorys[i];
    factorys[i].setStartX(x);
    x += factorys[i].getWidth();
    // add in space between groups
    if (((i + 1) % defaultGroupSizeSpace) == 0) { // <=
      x += margin * dataModel.getUnitDelimiterSize();
    }
  }
  width = x - margin * dataModel.getUnitDelimiterSize() + margin;
  layoutChanged();
}

PVS-Studio警告：

V6007表达式'（（（i + 1）％defaultGroupSizeSpace）== 0'始终为true。ByteViewerLayoutModel.java:66
V6048此表达式可以简化。该操作中的操作数“ defaultGroupSizeSpace”等于1。ByteViewerLayoutModel.java:66

十六进制字节查看器支持选择显示的组的大小：例如，您可以将输出配置为'ffff ffff'或'ff ff ff ff'格式。setFactorys方法负责这些组在用户界面中的位置。尽管自定义和显示工作正常，但此方法中的循环看起来非常可疑：除以1的余数始终为零，这意味着x坐标将在每次迭代时增加。怀疑在设置DataModel时添加属性和可用性groupSize。重构后还剩下垃圾吗？或者也许defaultGroupSizeSpace变量计算丢失了

？无论如何，尝试用dataModel.getGroupSize（）替换其值都会破坏布局，也许只有这段代码的作者才能给出明确的答案。

片段8：无效的验证，第2部分

private String parseArrayDimensions(String datatype,
                                    List<Integer> arrayDimensions) {
  String dataTypeName = datatype;
  boolean zeroLengthArray = false;
  while (dataTypeName.endsWith("]")) {
    if (zeroLengthArray) {                   // <=
      return null; // only last dimension may be 0
    }
    int rBracketPos = dataTypeName.lastIndexOf(']');
    int lBracketPos = dataTypeName.lastIndexOf('[');
    if (lBracketPos < 0) {
      return null;
    }
    int dimension;
    try {
      dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1,
                                                          rBracketPos));
      if (dimension < 0) {
        return null; // invalid dimension
      }
    }
    catch (NumberFormatException e) {
      return null;
    }
    dataTypeName = dataTypeName.substring(0, lBracketPos).trim();
    arrayDimensions.add(dimension);
  }
  return dataTypeName;
}

PVS-Studio警告：V6007表达式'zeroLengthArray'始终为false。 PdbDataTypeParser.java:278

此方法解析多维数组的维，并返回解析后剩余的文本或无效数据的null。一项验证检查旁边的注释指出，只有最后读取的大小可以为零。分析是从右到左进行的，因此可以理解，“ [0] [1] [2]”是有效的输入文本，而“ [2] [1] [0]”则不是。

但是，麻烦的是：没有人添加下一个大小为零的检查，并且解析器将吃掉无效数据而没有不必要的问题。您可能应该按以下方式修复try块：

try {
  dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1,
                                                      rBracketPos));
  if (dimension < 0) {
    return null; // invalid dimension
  } else if (dimension == 0) {
    zeroLengthArray = true;
  }
}
catch (NumberFormatException e) {
  return null;
}

自然地，有可能该效度标准随着时间的推移被证明是不必要的，或者作者的评论具有不同的含义，因此有必要检查阅读的第一维度。无论如何，数据验证是任何应用程序的关键部分，必须全权负责。其中的错误可能导致应用程序崩溃，并导致安全漏洞，数据泄漏，数据损坏或丢失（例如，如果在查询验证期间跳过SQL注入）。

关于其余的警告

读者可能会注意到发出了很多警告，但很少考虑。在项目中调整Cloc的方式不是很整齐，它计数了大约125万行Java代码（不是空的也没有注释）。事实是，几乎所有警告都极为相似：在这里，他们忘记了检查null，没有删除那里未使用的旧代码。我并不是真的想通过列出相同的东西来让读者感到厌倦，并且我在文章的开头提到了这种情况的一部分。

另一个示例是在错误使用子字符串方法的情况下的五十条警告“ V6009函数接收到一个奇数参数”（CParserUtils.java:280、ComplexName.java:48等）来获取任何分隔符之后的其余字符串。开发人员通常希望此分隔符出现在字符串中，而忘记了否则indexOf将返回-1，这是substring的不正确值。自然，如果数据不是从外部验证或接收的，则应用程序崩溃的可能性将大大降低。但是，总的来说，这些是我们希望摆脱的潜在危险场所。

结论

通常，Ghidra对代码的质量感到满意-没有明显的噩梦。该代码格式正确，样式非常一致：在大多数情况下，变量，方法以及其他所有内容都被赋予了明确的名称，可以找到解释性的注释，并且存在大量的测试。

自然地，没有问题，其中：

死代码，很可能在大量重构后仍然存在；
许多javadocs已经过时了，例如，指示不存在的参数。
使用IntelliJ IDEA时不可能方便开发;
围绕反射构建的模块化系统使导航项目和查找组件之间的依赖关系变得更加困难。

请不要忽视开发人员工具。像安全带一样，静态分析不是灵丹妙药，但是它可以帮助防止释放前造成的一些灾难。而且没有人喜欢使用登录的软件。

您可以在我们的博客中了解其他经过验证的项目。而且我们也有一个试用许可证和各种用于选择使用分析仪，无需为它付出。

如果您想与讲英语的读者分享这篇文章，请使用翻译链接：Nikita Lazeba。NSA，Ghidra和Unicorns。

NSA，Ghidra和独角兽

序幕