يتم إرسال PVS-Studio لـ Java إلى المسار. المحطة التالية هي Elasticsearch

بعيدًا عن السنة الأولى ، قام فريق PVS-Studio بالتدوين على عمليات التحقق من المشاريع مفتوحة المصدر بواسطة محلل الكود الثابت الذي يحمل نفس الاسم. حتى الآن ، تم فحص أكثر من 300 مشروع ، وتمت كتابة أكثر من 12000 حالة في قاعدة بيانات الأخطاء التي تم العثور عليها. في البداية ، تم تنفيذ محلل لاختبار رمز C و C ++ ، ثم ظهر دعم لغة C #. لذلك ، من بين المشروعات التي تم اختبارها ، تقع الأغلبية (> 80٪) على C و C ++. في الآونة الأخيرة ، تمت إضافة Java إلى اللغات المدعومة ، مما يعني أن PVS-Studio يفتح الأبواب أمام عالم جديد ، وقد حان الوقت لاستكمال قاعدة البيانات بأخطاء من مشاريع Java.

عالم Java ضخم ومتنوع ، لذا تتسع عيني عند اختيار مشروع لاختبار محلل جديد. في النهاية ، وقع الاختيار على محرك بحث النص الكامل وتحليلات Elasticsearch. هذا مشروع ناجح إلى حد ما ، وفي المشروعات الناجحة ، يكون اكتشاف الأخطاء مضاعفًا ، أو حتى ثلاثة أضعاف ، أكثر متعة. فما هي العيوب التي اكتشفها برنامج PVS-Studio لجافا؟ ستتم مناقشة نتيجة التحقق في المقالة.

التعارف السطحي مع Elasticsearch

Elasticsearch هو محرك البحث والتحليل النص الكامل مفتوح المصدر للتحجيم. يسمح لك بتخزين كميات كبيرة من البيانات وإجراء بحث وتحليلات سريعتين (في الوقت الفعلي تقريبًا). عادةً ما يتم استخدامه كآلية / تقنية أساسية توفر للتطبيقات وظائف معقدة ومتطلبات بحث.

ومن بين المواقع الرئيسية التي تستخدم Elasticsearch و Wikimedia و StumbleUpon و Quora و Foursquare و SoundCloud و GitHub و Netflix و Amazon و IBM و Qbox.

أعتقد مع التعارف يكفي.

كيف كان ذلك

لم تكن هناك مشاكل في التحقق. تسلسل الإجراءات بسيط للغاية ولم يستغرق الكثير من الوقت:

• تحميل 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); } .... }); .... } 

الخطأ في جزء التعليمات البرمجية هذا هو أنه إذا تعذر عليك قراءة السطر من المخزن المؤقت ، فستستدعي استدعاء الدالة startWith في حالة العبارة if NullPointerException . على الأرجح ، هذا خطأ مطبعي ، وعند كتابة الشرط ، كان المقصود من عامل التشغيل && بدلاً من || .

V6008 dereference خالية من "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 . تأخذ طريقة البدء العديد من وسيطات الإدخال ، بما في ذلك المشتبه به لدينا. بعد ذلك ، بناءً على التحقق من كائننا لاغية ، يتم تكوين كائن جديد ، يشارك في المنطق الإضافي للأسلوب. يخبرنا التحقق من وجود قيمة خالية أن followIndexMetadata لا يزال يمكن أن يأتي من الخارج باستخدام كائن فارغ. حسنا ، انظر أبعد من ذلك.

بعد ذلك ، يتم استدعاء طريقة التحقق من الصحة باستخدام الضغط على العديد من الوسائط (مرة أخرى ، من بينها الكائن المعني). وإذا نظرت إلى تنفيذ طريقة التحقق من الصحة ، فسيصبح كل شيء في مكانه الصحيح. يتم تمرير كائننا الخالي المحتمل بواسطة الوسيطة الثالثة إلى طريقة التحقق من الصحة ، حيث يتم إلغاء ترجمته دون قيد أو شرط. نتيجة لذلك ، 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(....); } .... }} 

لا يُعرف بالحجج التي تسمى طريقة البدء فعليًا. من الممكن أن يتم التحقق من كل الوسائط في مكان ما قبل استدعاء الأسلوب ، ولا نواجه أي إلغاء لإلغاء تحديد الكائن الخالي. ولكن ، يجب أن تعترف أن تطبيق التعليمات البرمجية لا يزال يبدو غير موثوق به ويستحق الاهتمام.

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 . تقول القاعدة: "يا شباب ، ماذا تفعل؟ كيف ذلك؟ يا مشكلة! لماذا تستخدم الكائن أولاً ، ثم في السطر التالي من التعليمات البرمجية تحقق من أنه لاغٍ ؟! " لذلك اتضح هنا dereferencing كائن فارغ. للأسف ، حتى تعليق أحد المطورين لم يساعد.

V6060 تم استخدام مرجع "السبب" قبل أن يتم التحقق منه ضد قيمة خالية. 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(); } .... } .... } 

تجدر الإشارة هنا إلى أن طريقة getCause للفئة Throwable يمكن أن تُرجع خالية . علاوة على ذلك ، يتم تكرار المشكلة المذكورة أعلاه ، وليس من المنطقي شرح شيء بالتفصيل.

شروط لا معنى لها

V6007 Expression 's.charAt (i)! =' \ T '' صحيح دائمًا. كرون جافا (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 'المتبقي == 0' خطأ دائمًا. 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 ، يمكن الإشارة إلى أن النسخة التي يتم نسخها ستكون دائمًا أقل من طول المفتاح . وبالتالي ، فإن المقارنة بين تساوي المتغير المتبقي مع 0 لا معنى لها وستعطي دائمًا نتيجة خاطئة ، وبالتالي لن يتم إنهاء الشرط من الحلقة. هل هذا الرمز يستحق الحذف ، أو هل تحتاج إلى إعادة النظر في منطق السلوك؟ أعتقد أن المطورين وحدهم سيكونون قادرين على تحديد كل ما لدي.

تعبير V6007 'healthCheckDn.indexOf (' = ')> 0' غير صحيح دائمًا. 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' غير صحيح دائمًا. BCrypt.java (429)
2. V6025 ربما يكون الفهرس '(int) x' خارج الحدود. BCrypt.java (431)

مشكلة N1. التعبير (int) x <0 خاطئ دائمًا (نعم ، نعم ، مرة أخرى V6007 ). لا يمكن أن يكون المتغير x سالبًا ، لأنه من النوع char . النوع 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); } 

الخطأ هنا هو أنه يتم مقارنة كائنات displaySize من النوع Integer عبر العامل == ، أي تتم مقارنتها بالرجوع إليها. من الممكن تمامًا مقارنة كائنات ColumnInfo ، حيث تحتوي حقول حجم الشاشة على روابط مختلفة ولكن لها نفس المحتوى. وفي هذه الحالة ، سوف تقدم لنا المقارنة نتيجة سلبية ، بينما توقعنا الحقيقة.

أود أن أقترح أن مثل هذه المقارنة يمكن أن تكون ناتجة عن فشل إعادة البناء ، وفي البداية كان حقل حجم الشاشة من النوع int .

V6058 تقارن وظيفة "يساوي" كائنات الأنواع غير المتوافقة: عدد صحيح ، TimeValue. 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())) // <= && ....) } 

ومرة أخرى مقارنة غير صحيحة من الكائنات. قارن الآن الكائنات التي أنواعها غير متوافقة ( عدد صحيح و TimeValue ). نتيجة هذه المقارنة واضحة ، وهي دائما خاطئة. يمكن ملاحظة أن حقول الفصل تتم مقارنتها بنفس الطريقة مع بعضها البعض ، من الضروري فقط تغيير أسماء الحقول. لذلك ، قرر المطور تسريع عملية كتابة التعليمات البرمجية مع نسخ لصق ، لكنه منح نفسه الخطأ. يقوم الفصل بتنفيذ أداة الحصول على حقل scrollSize ، لذا لتصحيح الخطأ ، سيكون الحل الصحيح هو استخدام الطريقة المناسبة: datafeed .getScrollSize () .

دعونا نلقي نظرة على أمثلة أخرى من الأخطاء دون أي تفسير. المشكلة واضحة بالفعل.

V6001 هناك تعبيرات فرعية متطابقة 'takeInMillis' إلى اليسار وإلى يمين العامل '=='. 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(); } 

miscellanea

V6006 تم تكوين العنصر ولكن لا يتم استخدامه. الكلمة المفتاحية "رمي" قد تكون مفقودة. JdbcConnection.java (88)

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

الخطأ واضح ولا يتطلب أي تفسير. ألقى المطور استثناءًا ، لكن بأي حال من الأحوال ألقى به أكثر من ذلك. سيتم إنشاء مثل هذا الاستثناء المجهول بنجاح ، وسيتم تدميره أيضًا بنجاح ، والأهم من ذلك ، دون أي أثر. والسبب هو عدم وجود بيان رمي .

V6003 استخدام نمط "if (A) {....} آخر إذا تم اكتشاف (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" الأولى على طريقة إرجاع. هذا يعني أن العبارة "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(....); } .... } 

يتم استخدام نفس الحالة مرتين على التوالي. الشرط الثاني لا لزوم له ، أم أنه من الضروري استخدام نوع مختلف بدلاً من Boolean ؟

وظيفة "السلسلة الفرعية" 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 بسرعة مع عالم جديد لنفسه ، نحتاج إلى اختبارات جديدة ومستخدمين جدد وتعليقات نشطة وعملاء :). لذا ، أقترح ، دون تأخير ، تنزيل واختبار محللنا على مسودة عملك!

إذا كنت ترغب في مشاركة هذه المقالة مع جمهور يتحدث الإنجليزية ، فالرجاء استخدام الرابط الخاص بالترجمة: مكسيم ستيفانوف. PVS-Studio لجافا يضرب الطريق. المحطة التالية هي Elasticsearch