PVS-Studio for Java已发送到路径。 下一站是Elasticsearch

图片1

从第一年开始,PVS-Studio团队就一直在用同名的静态代码分析器撰写关于开源项目检查的博客。 迄今为止,已经检查了300多个项目,并且已将超过12,000个案例写入发现的错误数据库。 最初,分析器用于测试C和C ++代码,然后出现了C#语言支持。 因此,在经过测试的项目中,大多数(> 80%)属于C和C ++。 最近,已将Java添加到受支持的语言中,这意味着PVS-Studio开启了通往新世界的大门,现在是时候用Java项目中的错误来补充数据库。

Java世界是巨大而多样的,因此在选择一个项目来测试新的分析器时,我会睁大眼睛。 最终,选择取决于全文搜索引擎和分析Elasticsearch。 这是一个相当成功的项目,在成功的项目中,发现错误会变得更加愉快,甚至是三倍甚至两倍。 那么,PVS-Studio为Java检测到哪些缺陷? 验证结果将在本文中讨论。

熟悉Elasticsearch


Elasticsearch是一个开源可扩展的全文本搜索和分析引擎。 它允许您存储大量数据,在其中进行快速搜索和分析(几乎实时)。 通常,它用作为应用程序提供复杂功能和搜索要求的基本机制/技术。

在使用Elasticsearch,Wikimedia,StumbleUpon,Quora,Foursquare,SoundCloud,GitHub,Netflix,Amazon,IBM,Qbox的主要站点中,均已注明。

我认为相识就足够了。

怎么样了


验证没有问题。 动作顺序非常简单,不需要花费很多时间:

  • GitHub下载Elasticsearch;
  • 我按照说明启动了Java分析器并启动了分析。
  • 我收到了分析器报告,对其进行了分析并重点介绍了一些有趣的案例。

现在,让我们说清楚。

注意事项 可能的NullPointerException


V6008空取消引用“行”。 GoogleCloudStorageFixture.java(451)

private static PathTrie<RequestHandler> defaultHandlers(....) { .... handlers.insert("POST /batch/storage/v1", (request) -> { .... // Reads the body line = reader.readLine(); byte[] batchedBody = new byte[0]; if ((line != null) || (line.startsWith("--" + boundary) == false)) // <= { batchedBody = line.getBytes(StandardCharsets.UTF_8); } .... }); .... } 

此代码段中的错误是,如果他们无法从缓冲区读取行,则在if语句的情况下调用startsWith方法将抛出NullPointerException 。 这很可能是拼写错误,在写条件时,用&&运算符代替||。

V6008'followIndexMetadata '的潜在空引用。 TransportResumeFollowAction.java(171),TransportResumeFollowAction.java(170),TransportResumeFollowAction.java(194)

 void start( ResumeFollowAction.Request request, String clusterNameAlias, IndexMetaData leaderIndexMetadata, IndexMetaData followIndexMetadata, ....) throws IOException { MapperService mapperService = followIndexMetadata != null // <= ? .... : null; validate(request, leaderIndexMetadata, followIndexMetadata, // <= leaderIndexHistoryUUIDs, mapperService); .... } 

V6008诊断的另一个警告。 现在,仔细研究一下followIndexMetadata对象。 start方法需要几个输入参数,包括我们的可疑对象。 然后,在检查我们的对象是否为null的基础上,形成了一个新对象,该对象参与了该方法的进一步逻辑。 检查null会告诉我们, followIndexMetadata仍然可以从外部使用null对象。 好的,再看看。

接下来,通过推入许多参数(再次,其中是所讨论的对象)来调用验证方法。 而且,如果您查看验证方法的实现,那么一切都准备就绪。 我们的潜在空对象由第三个参数传递给validate方法,在此方法无条件地取消引用。 结果,潜在的NullPointerException。

 static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex, // <= ....) { .... Map<String, String> ccrIndexMetadata = followIndex.getCustomData(....); // <= if (ccrIndexMetadata == null) { throw new IllegalArgumentException(....); } .... }} 

不知道启动方法实际调用了哪些参数。 可能在方法调用之前的某处检查了所有参数,并且我们不会遇到对null对象的任何取消引用。 但是,您必须承认,这样的代码实现仍然看起来不可靠并且值得关注。

V6060在验证是否为空之前,已使用“节点”引用。 RestTasksAction.java(152),RestTasksAction.java(151)

 private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNodes discoveryNodes, TaskInfo taskInfo) { .... DiscoveryNode node = discoveryNodes.get(nodeId); .... // Node information. Note that the node may be null because it has // left the cluster between when we got this response and now. table.addCell(fullId ? nodeId : Strings.substring(nodeId, 0, 4)); table.addCell(node == null ? "-" : node.getHostAddress()); table.addCell(node.getAddress().address().getPort()); table.addCell(node == null ? "-" : node.getName()); table.addCell(node == null ? "-" : node.getVersion().toString()); .... } 

另一个诊断规则在这里起作用,但是问题是相同的: NullPointerException 。 规则说:“伙计们,你在做什么? 怎么会这样 麻烦了! 为什么首先使用该对象,然后在下一行代码中将其检查为null ?!” 因此,事实证明这里取消了对空对象的引用。 ,即使其中一位开发人员的评论也无济于事。

V6060在验证是否为null之前,已使用“原因”参考。 StartupException.java(76),StartupException.java(73)

 private void printStackTrace(Consumer<String> consumer) { Throwable originalCause = getCause(); Throwable cause = originalCause; if (cause instanceof CreationException) { cause = getFirstGuiceCause((CreationException)cause); } String message = cause.toString(); // <= consumer.accept(message); if (cause != null) { // <= // walk to the root cause while (cause.getCause() != null) { cause = cause.getCause(); } .... } .... } 

这里应该注意, Throwable类的getCause方法可以返回null 。 此外,上述问题被重复,并且详细解释某些内容是没有意义的。

毫无意义的条件


V6007表达式's.charAt(i)!='\ T''始终为true。 Cron.java(1223)

 private static int findNextWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) { // intentionally empty } return i; } 

所考虑的函数从索引i开始返回第一个空格的索引。 怎么了 分析器发出警告, s.charAt(i)!='\ T'始终为真,这意味着将始终存在真相和表达式(s.charAt(i)!=''|| s.charAt(i)! ='\ t') 。 是这样吗 我认为您自己可以通过替换任何字符来轻松验证这一点。

结果,此方法将始终返回等于s.length()的索引,这是不正确的。 我敢于认为这是由于位置较高的方法造成的:

 private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) { // intentionally empty } return i; } 

我们实现了此方法,然后将其复制并进行了一些小的更正,以获取错误的findNextWhiteSpace方法。 方法已调整,已调整但未调整。 要解决这种情况,必须使用&&运算符而不是||。

V6007表达式'remaining == 0'始终为false。 PemUtils.java(439)

 private static byte[] generateOpenSslKey(char[] password, byte[] salt, int keyLength) { .... int copied = 0; int remaining; while (copied < keyLength) { remaining = keyLength - copied; .... copied += bytesToCopy; if (remaining == 0) { // <= break; } .... } .... } 

复制的周期的条件<keyLength可以看出, 复制的总是小于keyLength 。 因此,将剩余变量与0的相等性进行比较是没有意义的,并且总是给出错误的结果,因此该条件不会从循环中退出。 此代码是否值得删除,还是仍然需要重新考虑行为逻辑? 我认为只有开发人员才能将所有i点缀起来。

V6007表达式' healthCheckDn.indexOf ('=')> 0'始终为false。 ActiveDirectorySessionFactory.java(73)

 ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException { super(...., () -> { if (....) { final String healthCheckDn = ....; if (healthCheckDn.isEmpty() && healthCheckDn.indexOf('=') > 0) { return healthCheckDn; } } return ....; }, ....); .... } 

再次是毫无意义的表达。 根据条件,为了使lambda表达式返回字符串变量healthCheckDn ,字符串healthCheckDn必须都为空,并且包含字符'='的字符串不在第一位置。 嗯,有点整理了。 正如您正确理解的那样,这是不可能的。 我们不会理解代码的逻辑,将代码留给开发人员。

我仅给出了一些错误的示例,但除此之外,还有很多情况下触发V6007诊断程序,必须将它们分开考虑并得出结论。

方法虽小,但大胆


 private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; } 

因此,我们有一个几行的微小方法。 但是虫子不睡觉! 对这种方法的分析得出以下结果:

  1. V6007表达式'(int)x <0'始终为false。 BCrypt.java(429)
  2. V6025可能索引'(int)x'超出范围。 BCrypt.java(431)

问题N1。 表达式(int)x <0始终为false(是,是,再次是V6007 )。 变量x不能为负,因为它的类型为char字符类型是无符号整数。 这不能称为真正的错误,但是尽管如此,检查还是多余的,可以删除。

问题N2。 数组的可能溢出导致ArrayIndexOutOfBoundsException 。 然后,问题浮出水面:“等等,如何检查索引?”

因此,我们有一个包含128个元素的固定大小的数组:

 private static final byte index_64[] = { -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, -1, -1, -1, -1, -1, -1, -1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, -1, -1, -1, -1, -1, -1, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, -1, -1, -1, -1, -1 }; 

当变量x输入char64方法的输入时,它将检查索引的有效性。 差距在哪里? 为什么仍然有可能退出阵列?

检查(int)x> index_64.length并不完全正确。 如果x的值为128 到达char64方法的输入,则该检查将无法防止ArrayIndexOutOfBoundsException 。 也许这在现实中永远不会发生。 但是,检查的拼写不正确,您需要将运算符“更多”(>)替换为“更多或等于”(> =)。

尝试过的比较


V6013数字“ displaySize”和“ that.displaySize”通过引用进行比较。 可能希望进行平等比较。 ColumnInfo.java(122)

 .... private final String table; private final String name; private final String esType; private final Integer displaySize; .... @Override public boolean equals(Object o) { if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } ColumnInfo that = (ColumnInfo) o; return displaySize == that.displaySize && // <= Objects.equals(table, that.table) && Objects.equals(name, that.name) && Objects.equals(esType, that.esType); } 

这里的不正确之处在于,通过==运算符比较了Integer类型的displaySize对象,即,它们是通过引用进行比较的。 很可能会比较ColumnInfo对象,为此, displaySize字段具有不同的链接但内容相同。 在这种情况下,比较会给我们带来负面的结果,而我们却期待着事实。

我敢建议这种比较可能是重构失败的结果,并且最初的displaySize字段是int类型。

V6058 “等于”功能比较不兼容类型的对象:整数,时间值。 DatafeedUpdate.java(375)

 .... private final TimeValue queryDelay; private final TimeValue frequency; .... private final Integer scrollSize; .... boolean isNoop(DatafeedConfig datafeed) { return (frequency == null || Objects.equals(frequency, datafeed.getFrequency())) && (queryDelay == null || Objects.equals(queryDelay, datafeed.getQueryDelay())) && (scrollSize == null || Objects.equals(scrollSize, datafeed.getQueryDelay())) // <= && ....) } 

再次不正确地比较对象。 现在比较类型不兼容的对象( IntegerTimeValue )。 这种比较的结果是显而易见的,并且总是错误的。 可以看出,类的字段以相同的方式相互比较,只需要更改字段名称即可。 因此,开发人员决定加快使用复制粘贴编写代码的过程,但他还是给了自己同样的错误。 该类为scrollSize字段实现了一个吸气剂,因此,要更正错误,正确的解决方案是使用适当的方法: datafeed .getScrollSize()

让我们看更多错误示例,无需任何解释。 问题已经很明显了。

V6001在'=='运算符的左侧和右侧有相同的子表达式'tookInMillis'。 TermVectorsResponse.java(152)

 @Override public boolean equals(Object obj) { .... return index.equals(other.index) && type.equals(other.type) && Objects.equals(id, other.id) && docVersion == other.docVersion && found == other.found && tookInMillis == tookInMillis // <= && Objects.equals(termVectorList, other.termVectorList); } 

V6009函数“等于”收到一个奇数参数。 对象“ shardId.getIndexName()”用作其自身方法的参数。 SnapshotShardFailure.java(208)

 @Override public boolean equals(Object o) { .... return shardId.id() == that.shardId.id() && shardId.getIndexName().equals(shardId.getIndexName()) && // <= Objects.equals(reason, that.reason) && Objects.equals(nodeId, that.nodeId) && status.getStatus() == that.status.getStatus(); } 

杂项


V6006已创建对象, 但未使用该对象。 'throw'关键字可能丢失。 JdbcConnection.java(88)

 @Override public void setAutoCommit(boolean autoCommit) throws SQLException { checkOpen(); if (!autoCommit) { new SQLFeatureNotSupportedException(....); } } 

该错误是显而易见的,不需要解释。 开发人员引发了异常,但绝没有进一步引发异常。 这样的匿名异常将被成功创建,并且也将成功并且最重要的是销毁无踪。 原因是缺少throw语句。

V6003检测到使用'if(A){....} else if(A){....}'模式。 存在逻辑错误的可能性。 MockScriptEngine.java(94),MockScriptEngine.java(105)

 @Override public <T> T compile(....) { .... if (context.instanceClazz.equals(FieldScript.class)) { .... } else if (context.instanceClazz.equals(FieldScript.class)) { .... } else if(context.instanceClazz.equals(TermsSetQueryScript.class)) { .... } else if (context.instanceClazz.equals(NumberSortScript.class)) .... } 

在多if-else构造中,条件之一重复两次,因此这种情况需要对代码进行充分的审查。

V6039有两个带有相同条件表达式的'if'语句。 第一个'if'语句包含方法return。 这意味着第二个“ if”语句是毫无意义的。 SearchAfterBuilder.java(94),SearchAfterBuilder.java(93)

 public SearchAfterBuilder setSortValues(Object[] values) { .... for (int i = 0; i < values.length; i++) { if (values[i] == null) continue; if (values[i] instanceof String) continue; if (values[i] instanceof Text) continue; if (values[i] instanceof Long) continue; if (values[i] instanceof Integer) continue; if (values[i] instanceof Short) continue; if (values[i] instanceof Byte) continue; if (values[i] instanceof Double) continue; if (values[i] instanceof Float) continue; if (values[i] instanceof Boolean) continue; // <= if (values[i] instanceof Boolean) continue; // <= throw new IllegalArgumentException(....); } .... } 

同一条件连续使用两次。 第二个条件是多余的,还是有必要使用其他类型而不是布尔值

V6009函数“子字符串”接收奇数个参数。 'queryStringIndex + 1'参数不应大于'queryStringLength'。 LoggingAuditTrail.java(660)

 LogEntryBuilder withRestUriAndMethod(RestRequest request) { final int queryStringIndex = request.uri().indexOf('?'); int queryStringLength = request.uri().indexOf('#'); if (queryStringLength < 0) { queryStringLength = request.uri().length(); } if (queryStringIndex < 0) { logEntry.with(....); } else { logEntry.with(....); } if (queryStringIndex > -1) { logEntry.with(...., request.uri().substring(queryStringIndex + 1,// <= queryStringLength)); // <= } .... } 

立即考虑可能引发StringIndexOutOfBoundsException的错误情况。 当request.uri()返回包含早于'?'的字符'#'的字符串时,将发生异常。 对于这种情况,该方法中没有检查,如果仍然发生,则将无法避免灾难。 也许由于方法外对请求对象的各种检查,这种情况永远不会发生,但是我认为,这并不是最好的主意。

结论


多年来,PVS-Studio有助于发现商业和免费开源项目代码中的缺陷。 最近,添加了Java以支持解析的语言。 对于我们的新手来说,第一个测试是积极开发Elasticsearch。 我们希望这项检查对项目有用,对读者也有帮助。

为了使PVS-Studio for Java能够快速适应自己的新世界,我们需要新的测试,新的用户,有效的反馈和客户:)。 因此,我建议您立即下载并在您的工作草案中测试我们的分析仪!



如果您想与讲英语的人分享这篇文章,请使用以下链接:Maxim Stefanov。 PVS-Studio for Java上路了。 下一站是Elasticsearch

Source: https://habr.com/ru/post/zh-CN445682/


All Articles