nas和nsa_NSA，Ghidra和独角兽

nas和nsa

This time, the PVS-Studio team's attention was attracted by Ghidra, a big bad reverse-engineering framework allowing developers to analyze binary files and do horrible things to them. The most remarkable fact about it is not even that it's free and easily extensible with plugins but that it was developed and uploaded to GitHub for public access by NSA. On the one hand, you bet NSA has enough resources for keeping their code base clean. On the other hand, new contributors, who are not well familiar with it, may have accidentally introduced bugs that could stay unnoticed. So, we decided to feed the project to our static analyzer and see if it has any code issues.

这次，PVS-Studio团队的注意力被Ghidra吸引，Ghidra是一个糟糕的逆向工程框架，使开发人员可以分析二进制文件并对它们进行可怕的处理。 关于它的最引人注目的事实甚至不是它是免费且可轻松通过插件扩展的，而是它已开发并上传到GitHub以供NSA公开访问。 一方面，您打赌NSA有足够的资源来保持其代码库的整洁。 另一方面，对它不太熟悉的新贡献者可能偶然引入了可能不被注意的错误。 因此，我们决定将项目提供给我们的静态分析器，看看它是否存在任何代码问题。

序曲 (Overture)

PVS-Studio issued a total of 651 high-level, 904 medium-level, and 909 low-level warnings on the Java part of Ghidra (release 9.1.2, commit 687ce7f). About half of the high-level and medium-level ones were "V6022 Parameter is not used inside method's body" warnings, which typically show up after refactoring, when some parameter becomes no longer needed or some feature is temporarily commented out. A quick look through these warnings (there are too many of them to closely examine each as a third-party reviewer) didn't reveal anything outright suspicious. I guess it would be OK to turn off this diagnostic for the project in the analyzer's settings at least temporarily so that it doesn't distract developers. In general practice, though, you don't want to neglect it because setters' or constructors' parameters often have typos in their names. I'm sure most of you have seen an annoying pattern like this at least once:

PVS-Studio在Ghidra的Java部分( 版本9.1.2，提交687ce7f )上总共发出了651个高级，904个中级和909个低级警告。 在高级和中级警告中，大约有一半是“方法主体内部未使用V6022参数”警告，该警告通常在重构后出现，当不再需要某些参数或暂时注释掉某些功能时。 快速浏览这些警告(它们太多了，无法作为第三方审阅者进行仔细检查)并没有发现任何可疑的东西。 我想可以至少在分析器的设置中暂时关闭该项目的诊断，以免分散开发人员的注意力。 但是，在一般实践中，您不希望忽略它，因为设置器或构造函数的参数名称经常带有拼写错误。 我敢肯定，你们中的大多数人至少一次见过这种令人讨厌的模式：

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

More than half of the low-level warnings were produced by the "V6008 Potential null dereference of 'variable'" diagnostic – for example, the value returned by File.getParentFile() is often used without a prior null check. If the file object the method is called on was constructed without the absolute path, the method will return null, which may cause the application to crash if the check is missing.

一半以上的低级警告是由“变量的V6008潜在的空引用取消”诊断产生的，例如， File.getParentFile()返回的值通常在没有事先进行空检查的情况下使用。 如果在没有绝对路径的情况下构造了调用该方法的文件对象，则该方法将返回null ，如果缺少检查，则可能导致应用程序崩溃。

As usual, I'll be discussing only high-level and medium-level warnings since they usually reveal the major portion of actual bugs found in the project. When you deal with analysis reports, we recommend always going through the warnings in descending order of their certainty.

像往常一样，我将仅讨论高级和中级警告，因为它们通常会揭示项目中发现的实际错误的主要部分。 当您处理分析报告时，我们建议始终按照确定性从高到低的顺序进行警告。

Below I'll share several analyzer-reported code snippets, which I found suspicious or interesting. The large size of Ghidra's code base makes it almost impossible to find these defects manually.

在下面，我将分享一些分析器报告的代码片段，这些片段令人怀疑或有趣。 Ghidra的代码库很大，几乎不可能手动找到这些缺陷。

片段1：验证无效 (Snippet 1: broken validation)

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 warning: V6001 There are identical sub-expressions 'newDataType.getLength()' to the left and to the right of the '>' operator. DataTypeSelectionEditor.java:366

PVS-Studio警告： V6001 “>”运算符的左侧和右侧有相同的子表达式“ newDataType.getLength()”。 DataTypeSelectionEditor.java:366

This class provides a UI component for selecting data types with autocompletion support. The developer can specify the maximum size of the selectable data type (using the maxSize field) or make it unlimited by specifying a negative value. Should the limit be exceeded, the check of input data is supposed to throw an exception, which will be caught further up in the call stack, with the user getting an error message accordingly.

此类提供了一个UI组件，用于选择具有自动补全支持的数据类型。 开发人员可以指定可选数据类型的最大大小(使用maxSize字段)，也可以通过指定负值使其不受限制。 如果超出了限制，则应该检查输入数据以引发异常，该异常将在调用堆栈中进一步捕获，并且用户会相应地收到错误消息。

It seems the author of this code got distracted right in the middle of writing that check – or maybe they just started meditating upon the purpose of life and all. Whatever it was, they ended up with broken validation: the condition checks if the value can be greater than itself, and since it never can, the check is ignored. And that means the component could provide invalid data.

似乎在编写此检查的过程中，该代码的作者分心了–也许他们只是开始沉思于生命和一切目的。 不管是什么，它们最终都会导致验证失败：条件检查该值是否可以大于自身值，并且由于永远不能大于该值，因此将忽略该检查。 这意味着该组件可能会提供无效数据。

Another similar mistake was found in two other classes: GuidUtil and NewGuid.

在其他两个类中发现了另一个类似的错误： 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 warning: V6007 Expression 'data[i] != 0xFFFFFFFFL' is always true. GuidUtil.java:200

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

The condition in the for loop of the isOK method checks if a value is not equal to two different values at the same time. If it's true, the GUID will be acknowledged as valid right away. Thus, the GUID will be acknowledged as invalid only when the data array is empty, which will never be the case because the variable in question gets its value only once – at the beginning of the parseLine method.

isOK方法的for循环中的条件将同时检查一个值是否等于两个不同的值。 如果为真，则GUID将立即被确认为有效。 因此，只有在数据数组为空时，GUID才被确认为无效，这将永远不会发生，因为相关的变量仅在parseLine方法的开头获得其值一次。

The isOK method has the same body in both classes, which makes me think it's just another case of cloning incorrect code using copy-paste. I'm not sure what exactly the author wanted to check, but I'd say this method should be fixed as follows:

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：隐藏异常 (Snippet 2: hiding exceptions)

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 warning: V6006 The object was created but it is not being used. The 'throw' keyword could be missing: 'new MemoryAccessException(«Cyclic Access»)'. BitMappedSubMemoryBlock.java:99

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

As is widely known, exception objects don't do anything by themselves (at least they shouldn't). New instances are almost always thrown using the throw statement, and in certain rare cases they are passed somewhere or stored in a collection.

众所周知，异常对象本身不会做任何事情(至少它们不应该做)。 新实例几乎总是使用throw语句抛出 ，在极少数情况下，它们会传递到某个地方或存储在集合中。

The class the method above belongs to is a wrapper over a memory block allowing the developer to read and write data. Since the method doesn't throw exceptions, the restriction imposed on accessing the current memory block using the ioPending flag may get violated, and, furthermore, AddressOverflowException is ignored. That's how the data may get spoiled and you'll never know about it; instead of getting an explicit error message in a particular spot, you'll get strange artifacts that you'll have to debug.

上面的方法所属的类是存储块上的包装器，允许开发人员读取和写入数据。 由于该方法不会引发异常，所以可能会违反使用ioPending标志访问当前内存块所施加的限制，此外， AddressOverflowException也将被忽略。 这样一来，数据可能会被破坏，您将永远不会知道。 而不是在特定位置收到明确的错误消息，您将得到必须调试的奇怪工件。

There was a total of eight missing exceptions like that:

总共有八个这样的遗漏异常：

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

Curiously, those files also contain methods that look very much like the method above but do have throw in them. I suspect the programmer made a mistake in the original method, similar to the one discussed, cloned it a few times, and eventually discovered the mistake and fixed every clone of the method that they could remember of.

奇怪的是，这些文件还包含看起来非常像上面的方法，但确实有他们投掷方法。 我怀疑程序员在原始方法中犯了一个错误，类似于所讨论的方法，将其克隆了几次，最终发现了错误并修复了他们可以记住的方法的每个克隆。

片段3：雷区 (Snippet 3: minefield)

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 warning: V6008 Null dereference of 'selectedNode' in function 'setViewPanel'. OptionsPanel.java:266

PVS-Studio警告： V6008函数“ setViewPanel”中“ selectedNode”的空引用已取消。 选项面板.java：266

The analyzer isn't entirely correct about this one: the call of the processSelection method won't currently lead to NullPointerException as this method is called only twice, with an explicit null check of selectedNode before the call. That's not a good thing to do because another developer may assume that since the method explicitly handles the selectedNode == null case, this value should be valid and use it, potentially ending up with a crash. This is particularly true for open-source projects because contributing developers may not be well familiar with the code base.

分析器对此并不完全正确： processSelection方法的调用当前不会导致NullPointerException，因为此方法仅被调用了两次，并且在调用之前对selectedNode进行了显式的null检查。 这不是一件好事，因为另一个开发人员可能会认为，由于该方法显式处理了selectedNode == null的情况，因此该值应该是有效的并可以使用它，最终可能导致崩溃。 对于开源项目尤其如此，因为贡献开发人员可能对代码库不太熟悉。

Actually, the entire processSelection method looks strange. This might well be a copy-paste error because you can also find two if blocks with identical bodies but different conditions further down in this same method. However, selectedNode won't be null by that time and the entire call chain setViewPanel-setHelpLocation won't lead to a NullPointerException.

实际上，整个processSelection方法看起来很奇怪。 这很可能是复制粘贴错误，因为在同一方法中，您还可以找到两个具有相同主体但条件不同的if块。 但是，届时selectedNode不会为null ，整个调用链setViewPanel-setHelpLocation不会导致NullPointerException 。

片段4：有害的自动补全 (Snippet 4: harmful autocompletion)

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 warning: V6053 The collection is modified while the iteration is in progress. ConcurrentModificationException may occur. DWARFExpressionOpCodes.java:205

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

Again, the analyzer is not completely right: the exception will not be thrown because the UNSUPPORTED_OPCODES collection is always empty and the loop just won't execute at all. Besides, this collection happens to be a set, which means adding an already existing element won't affect it. The programmer must have entered the collection name in the for-each loop using autocompletion, which suggested the wrong field, with the programmer never noticing that. A collection can't be modified while it is being iterated, but if you are lucky enough – as in this case – the application won't crash. In this snippet, the typo affects the program indirectly: the DWARF file parser relies on the collection to stop analysis when encountering unsupported opcodes.

同样，分析器也不完全正确：不会抛出异常，因为UNSUPPORTED_OPCODES集合始终为空，并且循环根本不会执行。 此外，此集合恰好是一个集合，这意味着添加一个已经存在的元素不会影响它。 程序员必须使用自动完成功能在for-each循环中输入了集合名称，这暗示了错误的字段，而程序员从未注意到这一点。 迭代时无法修改集合，但是如果您足够幸运(在这种情况下)，应用程序将不会崩溃。 在此代码段中，拼写错误会间接影响程序：DWARF文件解析器在遇到不受支持的操作码时将依赖于该集合来停止分析。

Starting with Java 9, it's better to use factory methods of the standard library to create constant collections: for example, Set.of(T… elements) is not only much more convenient to use but also makes the newly created collection immutable from the start, thus enhancing reliability.

从Java 9开始，最好使用标准库的工厂方法来创建常量集合：例如， Set.of(T…elements)不仅使用起来更加方便，而且使新创建的集合从一开始就不可变 ，从而提高可靠性。

片段5：全部都可以找到 (Snippet 5: all shall be found)

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:

PVS-Studio警告：

• V6007 Expression 'index >= 0' is always true. ExternalNamesTableModel.java:105

  V6007表达式'index> = 0'始终为true。 ExternalNamesTableModel.java:105

• V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java:109

  V6019检测不到代码。 可能存在错误。 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.

某些东西使开发人员分心，他们意外地实现了indexOf方法，该方法返回0，即path集合的第一个元素的索引，而不返回-1(不存在的值)。 即使集合为空，也会发生这种情况。 或者，也许他们生成了该方法，但却忘记了更改默认返回值。 无论如何，即使集合中没有单个名称， setValueAt方法也会拒绝任何提供的值并显示消息“名称已存在”。

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.

顺便说一句，在其他任何地方都没有使用indexOf方法，实际上仅需要它的值来确定所寻找的元素是否存在。 与其编写单独的方法并处理索引，不如直接在setValueAt方法中编写一个for-each循环，并在遇到匹配元素时返回它，可能会更好。

Note: I didn't manage to reproduce the assumed bug. Perhaps the setValueAt method is no longer used or is called only under certain conditions.

注意：我没有设法重现假定的错误。 可能不再使用setValueAt方法或仅在某些条件下才调用setValueAt方法。

片段6：请安静 (Snippet 6: quiet please)

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 warning: V6033 An item with the same key '@' has already been added. FilterOptions.java:45

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

Ghidra supports data filtering in different contexts. For example, you can filter project files by name. Filtering by several keywords at a time is also supported – specifying '.java,.c' with 'OR' mode enabled will display all files whose names contain either '.java' or '.c'. Theoretically, you can use any special character as a separator (it's selected in the filter settings), but in reality the exclamation point is not available.

Ghidra支持在不同上下文中进行数据过滤。 例如，您可以按名称过滤项目文件。 还支持一次按几个关键字进行过滤-在启用“或”模式的情况下指定“ .java，.c”将显示名称包含“ .java”或“ .c”的所有文件。 从理论上讲，您可以使用任何特殊字符作为分隔符(在过滤器设置中选择该字符)，但实际上感叹号不可用。

It's very easy to make a typo in long initialization lists like that because they are often created using copy-paste, and your attention weakens quickly when reviewing such code manually. If the duplicate lines are not adjacent, there's almost no chance of noticing the typo through manual review.

在很长的初始化列表中输入这样的错字很容易，因为它们通常是使用复制粘贴创建的，并且当您手动查看此类代码时，注意力会很快减弱。 如果重复的行不相邻，则几乎没有机会通过人工检查发现错字。

片段7：余数始终为0 (Snippet 7: remainder is always 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 warnings:

PVS-Studio警告：

• V6007 Expression '((i + 1) % defaultGroupSizeSpace) == 0' is always true. ByteViewerLayoutModel.java:66

  V6007表达式'(((i + 1)％defaultGroupSizeSpace)== 0'始终为true。 ByteViewerLayoutModel.java:66

• V6048 This expression can be simplified. Operand 'defaultGroupSizeSpace' in the operation equals 1. ByteViewerLayoutModel.java:66

  V6048此表达式可以简化。 该操作中的操作数“ defaultGroupSizeSpace”等于1。ByteViewerLayoutModel.java:66

The hex byte viewer allows selecting the length of groups to be displayed: for example, you can set the data output format as 'ffff ffff' or 'ff ff ff ff'. The setFactorys method is responsible for arranging these groups in the UI. While both the customization and representation are fine here, the loop in this method doesn't look right at all: the remainder of division by one is always 0, which means the x coordinate will be increased at each iteration. The fact that the dataModel parameter has a groupSize property makes it even more suspicious.

十六进制字节查看器允许选择要显示的组的长度：例如，您可以将数据输出格式设置为“ ffff ffff”或“ ff ff ff ff”。 setFactorys方法负责在UI中安排这些组。 尽管自定义和表示都很好，但是此方法中的循环看起来并不正确：除以1的余数始终为0，这意味着x坐标将在每次迭代时增加。 dataModel参数具有groupSize属性的事实使它更加可疑。

Are these some refactoring leftovers? Or maybe some calculations of the defaultGroupSizeSpace variable are missing? In any case, attempting to replace its value with dataModel.getGroupSize() results in the broken layout, and only the author of this code could tell us what it's all about.

这些是重构剩余吗？ 或者也许缺少对defaultGroupSizeSpace变量的一些计算？ 无论如何，尝试用dataModel.getGroupSize()替换其值都会导致布局损坏，并且只有这段代码的作者才能告诉我们所有内容。

片段8：无效的验证，第2部分 (Snippet 8: broken validation, part 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 warning: V6007 Expression 'zeroLengthArray' is always false. PdbDataTypeParser.java:278

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

This method parses the dimensions of a multi-dimensional array and returns either the remaining text or null for invalid data. The comment next to one of the validity checks says only the last dimension read may be 0. The parsing runs from right to left, which means '[0][1][2]' is valid input text and '[2][1][0]' is not.

此方法解析多维数组的维，并为无效数据返回其余文本或null 。 一项有效性检查旁边的注释说，仅最后读取的维度可能为0。解析过程是从右向左进行的，这意味着“ [0] [1] [2]”是有效的输入文本，“ [2] [ 1] [0]'不是。

But, unfortunately, the loop has no condition to check if the read dimension is 0, so the parser consumes invalid data without any questions. I think it should be fixed by modifying the try block as follows:

但是，不幸的是，循环没有条件检查读取的维数是否为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;
}

Sure, this validity criteria may have eventually turned out to be irrelevant or the programmer meant something else by their comment and it's the first read dimension that should be checked. In any case, data validation is a critical spot of any application, and you must take it seriously. Mistakes in it may cause both relatively harmless crashes and severe security holes, data leaks, data corruption, or data loss (overlooking an SQL injection during request validation is one such example).

当然，这个有效性标准可能最终不相关，或者程序员的注释意味着其他含义，这是应检查的第一个读取维度。 无论如何，数据验证是任何应用程序的关键所在，您必须认真对待。 错误会导致相对无害的崩溃和严重的安全漏洞，数据泄漏，数据损坏或数据丢失(在请求验证期间忽略SQL注入就是这样的示例)。

关于其他警告的几句话 (A few words about other warnings)

You may have noticed that while there were many warnings issued, I've actually shared just a few of them. The cloc utility, which I didn't bother to fine tune, counted about 1.25 million lines of Java code (blank lines and comments excluded). It's just that almost all of these warnings look very much the same: some deal with missing null checks, others with legacy code no longer needed but still present. I don't want to bore you by showing you the same types of bugs over and over again, and I did mention some of them in the beginning.

您可能已经注意到，尽管发布了许多警告，但实际上我只分享了其中一些警告。 我不费吹灰之力地使用cloc实用程序，它计数了大约125万行Java代码(不包括空白行和注释)。 只是几乎所有这些警告看起来都差不多：一些警告处理缺少空检查，另一些警告则不再需要但仍然存在。 我不想一遍又一遍地向您展示相同类型的错误，这很无聊，我一开始确实提到了其中一些错误。

To show you one last example, let's talk about half a hundred "V6009 Function receives an odd argument" warnings in the context of unsafe usage of the substring method (CParserUtils.java:280, ComplexName.java:48, etc.) to get the part of a string following a separator. Developers often hope for the separator to be present in the string and forget that indexOf will return -1 otherwise, which is an incorrect value for substring. Of course, if the data has been validated or doesn't come from «the outside», the program will be much less likely to crash. But generally, these defects are potentially unsafe and we want to help eliminate them.

为了向您展示最后一个示例，让我们来谈谈在不安全使用子字符串方法(CParserUtils.java:280、ComplexName.java:48等)的情况下的一百个“ V6009函数接收到一个奇怪的参数”警告字符串后面的分隔符部分。 开发人员通常希望分隔符出现在字符串中，而忘了indexOf否则将返回-1，这对于substring来说是不正确的值。 当然，如果数据已经过验证或不是来自“外部”，则程序崩溃的可能性将大大降低。 但通常，这些缺陷可能不安全，我们希望帮助消除它们。

结论 (Conclusion)

The overall quality of Ghidra is pretty high – there are no outright screwups, which is cool. The code is mostly well formatted and the style is quite consistent: most of the variables, methods, and other entities have intelligible names, certain details are cleared up through comments, and the number of tests is huge.

吉德拉(Ghidra)的整体质量相当高-没有彻底的搞砸，这很酷。 代码大部分都具有良好的格式，并且样式也非常一致：大多数变量，方法和其他实体的名称都易于理解，某些详细信息通过注释清除，并且测试数量庞大。

Of course, there were some issues, including the following:

当然，有一些问题，包括：

• Dead-code fragments, which must be leftovers from multiple refactoring sessions;
  死代码片段，必须是来自多个重构会话的剩余片段；
• Many of the javadocs are long obsolete and, for example, point at non-existent parameters;
  许多Javadocs早已过时，例如，指向不存在的参数。

• Convenient development using IntelliJ IDEA is unsupported;

  不支持使用IntelliJ IDEA进行方便的开发；

• The modular system built around reflection makes project navigation and component dependency search much, much harder.
  围绕反射构建的模块化系统使项目导航和组件依存关系搜索变得非常困难。

Please don't disregard developer tools. Static analysis, just like seat belts, is not a cure-all, but it helps prevent some of the accidents before the release. After all, no one likes using bug-teeming software, do they?

请不要忽略开发人员工具。 像安全带一样，静态分析并不是万能的，但可以帮助防止释放前发生的一些事故。 毕竟，没有人喜欢使用漏洞大量的软件，对吗？

Check our blog for other posts about projects we have checked. We also offer a trial license and a handful of options of using the analyzer without paying for it.

在我们的博客中查看有关我们已检查的项目的其他帖子。 我们还提供了试用许可证，并提供了一些无需付费即可使用分析仪的选择 。

翻译自: https://habr.com/en/company/pvs-studio/blog/504778/

nas和nsa