💆 👩‍👧 📣 PVS-Studio لجافا يضرب الطريق. المحطة التالية هي Elasticsearch 🤸🏾 🏂🏽 🕧

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

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

باختصار حول elasticsearch

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

من بين المواقع الرئيسية التي تستخدم Elasticsearch هناك ويكيميديا ، StumbleUpon ، Quora ، شخصيات قصص الابطال الخارقين ، 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 التشخيص. الكائن اتبعإندإكسميتاتا أشعل اهتمامي. تقبل طريقة البدء عدة حجج كمدخلات ، المشتبه به هو الصحيح فيما بينها. بعد ذلك ، استنادًا إلى التحقق من عدم وجود كائن لدينا ، يتم إنشاء كائن جديد ، يشارك في منطق الأسلوب الآخر. يُظهر التحقق من وجود قيمة خالية أنه لا يزال بإمكانك متابعةIndexMetadata من الخارج ككائن فارغ. حسنا ، دعنا ننظر إلى أبعد من ذلك.

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

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

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

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

تعبير 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 ....; }, ....); .... }

تعبير لا معنى له مرة أخرى. وفقًا للشرط ، يجب أن تكون سلسلة healthCheckDn فارغة وأن تحتوي على الحرف '=' ليس في الموضع الأول ، بحيث يُرجع تعبير lambda السلسلة متغير healthCheckDn . Phew ، هذا كل شيء بعد ذلك! كما فهمت ربما ، فمن المستحيل. لن نتعمق في الكود ، فلنترك الأمر لتقدير المطورين.

لقد ذكرت فقط بعض الأمثلة الخاطئة ، ولكن بعد ذلك كان هناك الكثير من عمليات التشخيص V6007 ، والتي ينبغي اعتبارها واحدة تلو الأخرى ، مما يجعل الاستنتاجات ذات الصلة.

طريقة صغيرة يمكن أن تقطع شوطا طويلا

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

حتى هنا لدينا طريقة صغيرة جدا من عدة خطوط. لكن البق على المراقبة! أعطى تحليل هذه الطريقة النتيجة التالية:

تعبير V6007 '(int) x <0' غير صحيح دائمًا. BCrypt.java (429)
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 };

عندما يستقبل أسلوب char64 المتغير x ، يتم التحقق من صحة الفهرس. أين هو العيب؟ لماذا لا يزال فهرس الصفيف خارج الحدود ممكنًا؟

علامة الاختيار (int) x> index_64.length غير صحيحة تمامًا. إذا كانت طريقة char64 تستقبل x بقيمة 128 ، فلن يحمي الشيك من 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 تتم مقارنتها باستخدام == عامل التشغيل ، أي حسب المرجع. من المحتمل تمامًا مقارنة كائنات ColumnInfo التي تحتوي حقول حجمها المعروضة على مراجع مختلفة ولكن نفس المحتوى. في هذه الحالة ، ستعطينا المقارنة النتيجة السلبية ، عندما توقعنا أن نحقق ذلك.

أود أن أخمن أن مثل هذه المقارنة يمكن أن تكون ناتجة عن فشل إعادة بيع المباني وفي البداية كان حقل displaySize من النوع 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(); }

متنوع

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)) .... }

في حالات متعددة - إذا تم تكرار أحد الشروط مرتين ، فإن الموقف يتطلب مراجعة الكود المختص.

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(....); } .... }

يتم استخدام نفس الحالة مرتين على التوالي. هل الشرط الثاني غير ضروري أم يجب استخدام نوع آخر بدلاً من المنطقية ؟

وظيفة "السلسلة الفرعية" 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