Clever Geek Handbook

PVS-Studioを使用したCUBAプラットフォームコード分析

写真2






Javaプログラマーには、強力なIntelliJ IDEA開発環境、無料のSpotBugs、PMDアナライザーなど、高品質のコードを作成するのに役立つ便利なツールがあります。 これらはすべてCUBAプラットフォームプロジェクトの開発ですでに使用されています。発見されたコードの欠陥のこのレビューでは、PVS-Studio静的コードアナライザーを使用してプロジェクトの品質を改善する方法を説明します。



プロジェクトとアナライザーについて



CUBAプラットフォームは、完全なWebインターフェイスを備えたエンタープライズアプリケーションを迅速に作成するための高レベルJavaフレームワークです。 このプラットフォームは、開発者を異種テクノロジーから抽象化するため、ビジネス上の問題を同時に解決することに集中することができます。 ソースコードはGitHubのリポジトリから取得されます 。



PVS-Studioは、C、C ++、C＃、およびJavaで記述されたプログラムのソースコードのエラーおよび潜在的な脆弱性を検出するためのツールです。 Windows、Linux、macOS上の64ビットシステムで動作します。 Javaプログラマーの便宜のために、Maven、Gradle、およびIntelliJ IDEAのプラグインを開発しました。 CUBA Platformプロジェクトは、Gradleプラグインを使用して簡単に検証されました。



条件のエラー



警告1



V6007式 'StringUtils.isNotEmpty（ "handleTabKey"）'は常にtrueです。 SourceCodeEditorLoader.java（60） 



@Override public void loadComponent() { .... String handleTabKey = element.attributeValue("handleTabKey"); if (StringUtils.isNotEmpty("handleTabKey")) { resultComponent.setHandleTabKey(Boolean.parseBoolean(handleTabKey)); } .... }


いくつかの要素から属性値を取得した後、この値はチェックされません。 代わりに、定数文字列がisNotEmpty関数に渡されますが、変数handleTabKeyを渡す必要がありました 。



AbstractTableLoader.javaファイルには、別の同様のエラーがあります。





警告2



V6007式 'previousMenuItemFlatIndex> = 0'は常にtrueです。 CubaSideMenuWidget.java（328） 



 protected MenuItemWidget findNextMenuItem(MenuItemWidget currentItem) { List<MenuTreeNode> menuTree = buildVisibleTree(this); List<MenuItemWidget> menuItemWidgets = menuTreeToList(menuTree); int menuItemFlatIndex = menuItemWidgets.indexOf(currentItem); int previousMenuItemFlatIndex = menuItemFlatIndex + 1; if (previousMenuItemFlatIndex >= 0) { return menuItemWidgets.get(previousMenuItemFlatIndex); } return null; }


indexOf関数は、アイテムがリストに見つからない場合、 -1を返すことができます。 次に、1つがインデックスに追加され、目的の要素が欠落している状況を隠します。 別の潜在的な問題は、変数previousMenuItemFlatIndexが常にゼロ以上になるという事実です。 たとえば、 menuItemWidgetsリストが空の場合、配列の境界を超えることが可能になります。



警告3



V6009 「削除」機能は「-1」値を受け取る可能性がありますが、負でない値が期待されます。 引数の検査：1. AbstractCollectionDatasource.java（556） 



 protected DataLoadContextQuery createDataQuery(....) { .... StringBuilder orderBy = new StringBuilder(); .... if (orderBy.length() > 0) { orderBy.delete(orderBy.length() - 2, orderBy.length()); orderBy.insert(0, " order by "); } .... }


文字バッファーorderByでは 、最後の2文字が合計数が0より大きい場合、つまり削除されます。 文字列には1文字以上が含まれます。 ただし、文字を削除するための開始位置は2文字のオフセットで設定されました。 したがって、 orderByが突然1文字で構成されている場合、削除しようとするとStringIndexOutOfBoundsExceptionがスローされます 。



警告4



V6013オブジェクト「masterCollection」と「entities」は参照により比較されます。 おそらく同等の比較が意図されていました。 CollectionPropertyContainerImpl.java（81） 



 @Override public void setItems(@Nullable Collection<E> entities) { super.setItems(entities); Entity masterItem = master.getItemOrNull(); if (masterItem != null) { MetaProperty masterProperty = getMasterProperty(); Collection masterCollection = masterItem.getValue(masterProperty.getName()); if (masterCollection != entities) { updateMasterCollection(masterProperty, masterCollection, entities); } } }


updateMasterCollection関数では、 エンティティの値がmasterCollectionにコピーされます 。 この前に、コレクションは参照によって比較されましたが、おそらく値によって比較される予定でした。



警告5



V6013オブジェクト 'value'および 'oldValue'は参照により比較されます。 おそらく同等の比較が意図されていました。 WebOptionsList.java（278） 



 protected boolean isCollectionValuesChanged(Collection<I> value, Collection<I> oldValue) { return value != oldValue; }


この例は、前の例に似ています。 isCollectionValuesChangedは、コレクションを比較するためのものです。 おそらく、参照による比較も予想された方法ではありません。



過剰条件



警告1



V6007式 'mask.charAt（i + offset）！= PlaceHolder'は常にtrueです。 DatePickerDocument.java（238） 



 private String calculateFormattedString(int offset, String text) .... { .... if ((mask.charAt(i + offset) == placeHolder)) { // <= .... } else if ((mask.charAt(i + offset) != placeHolder) && // <= (Character.isDigit(text.charAt(i)))) { .... } .... }


2番目の比較では、最初の比較とは反対の式がチェックされます。 2番目のチェックを削除すると、コードが簡素化されます。



V6007式 'connector == null'は常にfalseです。 HTML5Support.java（169） 



 private boolean validate(NativeEvent event) { .... while (connector == null) { widget = widget.getParent(); connector = Util.findConnectorFor(widget); } if (this.connector == connector) { return true; } else if (connector == null) { // <= return false; } else if (connector.getWidget() instanceof VDDHasDropHandler) { return false; } return true; }


whileループが完了した後、 コネクタ変数の値はnullにならないため、冗長なチェックを削除できます。



開発者が注意すべき別の不審な場所：





テストで到達不能なコード



V6019到達不能コードが検出されました。 エラーが存在する可能性があります。 TransactionTest.java（283） 



 private void throwException() { throw new RuntimeException(TEST_EXCEPTION_MSG); } @Test public void testSuspendRollback() { Transaction tx = cont.persistence().createTransaction(); try { .... Transaction tx1 = cont.persistence().createTransaction(); try { EntityManager em1 = cont.persistence().getEntityManager(); assertTrue(em != em1); Server server1 = em1.find(Server.class, server.getId()); assertNull(server1); throwException(); // <= tx1.commit(); // <= } catch (Exception e) { // } finally { tx1.end(); } tx.commit(); } finally { tx.end(); } }


throwException関数は、 tx1.commit関数の呼び出しを妨げる例外をスローします。 おそらくこれらの行を交換する必要があります。



他のテストのいくつかの類似した場所：





疑わしい関数の引数



警告1



V6023パラメーター 'salt'は、使用される前に常にメソッド本体で書き換えられます。 BCryptEncryptionModule.java（47） 



 @Override public String getHash(String content, String salt) { salt = BCrypt.gensalt(); return BCrypt.hashpw(content, salt); }


暗号化では、 ソルトはパスワードとともにハッシュ関数に渡されるデータの文字列です。 主に辞書検索やレインボーテーブルを使用した攻撃からの保護、および同じパスワードの非表示に使用されます。 続きを読む： ソルト（暗号）



この関数では、関数に入るとすぐに行が摩擦されます。 おそらく、渡された値を無視することは潜在的な脆弱性です。



警告2



考慮された機能について、アナライザーは一度に2つの警告を出します。 







 @Override public void setPosition(int offsetWidth, int offsetHeight) { offsetHeight = getOffsetHeight(); .... if (offsetHeight + getPopupTop() > ....)) { .... } .... offsetWidth = containerFirstChild.getOffsetWidth(); if (offsetWidth + getPopupLeft() > ....)) { .... } else { left = getPopupLeft(); } setPopupPosition(left, top); }


面白いコード。 この関数は、2つの変数offsetWidthとoffsetHeightのみを受け入れます。両方とも使用前に上書きされます。



警告3



V6022パラメーター 'shortcut'は、コンストラクター本体内では使用されません。 DeclarativeTrackingAction.java（47） 



 public DeclarativeTrackingAction(String id, String caption, String description, String icon, String enable, String visible, String methodName, @Nullable String shortcut, ActionsHolder holder) { super(id); this.caption = caption; this.description = description; this.icon = icon; setEnabled(enable == null || Boolean.parseBoolean(enable)); setVisible(visible == null || Boolean.parseBoolean(visible)); this.methodName = methodName; checkActionsHolder(holder); }


ショートカットパラメーターの値は、関数では使用されません。 関数インターフェースが古くなっているか、この警告はエラーではありません。



似たような場所：





同じコードで異なる機能



警告1



V6032メソッド 'firstItemId'の本文が別のメソッド 'lastItemId'の本文と完全に同等であることは奇妙です。 ContainerTableItems.java（213）、ContainerTableItems.java（219） 



 @Override public Object firstItemId() { List<E> items = container.getItems(); return items.isEmpty() ? null : items.get(0).getId(); } @Override public Object lastItemId() { List<E> items = container.getItems(); return items.isEmpty() ? null : items.get(0).getId(); }


関数firstItemIdとlastItemIdの実装は同じです。 後者では、インデックス0の要素を取得するのではなく、最後の要素のインデックスを計算する必要がありました。



警告2



V6032メソッドの本体が別のメソッドの本体と完全に同等であることは奇妙です。 SearchComboBoxPainter.java（495）、SearchComboBoxPainter.java（501） 



 private void paintBackgroundDisabledAndEditable(Graphics2D g) { rect = decodeRect1(); g.setPaint(color53); g.fill(rect); } private void paintBackgroundEnabledAndEditable(Graphics2D g) { rect = decodeRect1(); g.setPaint(color53); g.fill(rect); }


実装が疑わしい2つの機能。 そのうちの1 つでは 、 color53とは異なる色を使用する必要があることを提案しようと思います。



ヌル参照



警告1



V6060 「descriptionPopup」参照は、nullに対して検証される前に使用されました。 SuggestPopup.java（252）、SuggestPopup.java（251） 



 protected void updateDescriptionPopupPosition() { int x = getAbsoluteLeft() + WIDTH; int y = getAbsoluteTop(); descriptionPopup.setPopupPosition(x, y); if (descriptionPopup!=null) { descriptionPopup.setPopupPosition(x, y); } }


わずか2行で、著者は非常に疑わしいコードを書くことに成功しました。 最初に、 setPopupPositionメソッドがdescriptionPopupオブジェクトで呼び出され、次にオブジェクトがnullと比較されます 。 おそらく、 setPopupPosition関数の最初の呼び出しは冗長で危険です。 リファクタリングの失敗の結果のように見えます。



警告2



V6060 nullに対して検証される前に、「tableModel」参照が使用されました。 DesktopAbstractTable.java（1580）、DesktopAbstractTable.java（1564） 



 protected Column addRuntimeGeneratedColumn(String columnId) { // store old cell editors / renderers TableCellEditor[] cellEditors = new TableCellEditor[tableModel.getColumnCount() + 1]; // <= TableCellRenderer[] cellRenderers = new TableCellRenderer[tableModel.getColumnCount() + 1]; // <= for (int i = 0; i < tableModel.getColumnCount(); i++) { // <= Column tableModelColumn = tableModel.getColumn(i); if (tableModel.isGeneratedColumn(tableModelColumn)) { // <= TableColumn tableColumn = getColumn(tableModelColumn); cellEditors[i] = tableColumn.getCellEditor(); cellRenderers[i] = tableColumn.getCellRenderer(); } } Column col = new Column(columnId, columnId); col.setEditable(false); columns.put(col.getId(), col); if (tableModel != null) { // <= tableModel.addColumn(col); } .... }


同様の状況がこの関数に存在します。 tableModelオブジェクトを何度も呼び出した後、 nullかどうかを確認します 。



別の例：





おそらく論理的な間違い



V6026この値はすでに「sortAscending」変数に割り当てられています。 CubaScrollTableWidget.java（488） 



 @Override protected void sortColumn() { .... if (sortAscending) { if (sortClickCounter < 2) { // special case for initial revert sorting instead of reset sort order if (sortClickCounter == 0) { client.updateVariable(paintableId, "sortascending", false, false); } else { reloadDataFromServer = false; sortClickCounter = 0; sortColumn = null; sortAscending = true; // <= client.updateVariable(paintableId, "resetsortorder", "", true); } } else { client.updateVariable(paintableId, "sortascending", false, false); } } else { if (sortClickCounter < 2) { // special case for initial revert sorting instead of reset sort order if (sortClickCounter == 0) { client.updateVariable(paintableId, "sortascending", true, false); } else { reloadDataFromServer = false; sortClickCounter = 0; sortColumn = null; sortAscending = true; client.updateVariable(paintableId, "resetsortorder", "", true); } } else { reloadDataFromServer = false; sortClickCounter = 0; sortColumn = null; sortAscending = true; client.updateVariable(paintableId, "resetsortorder", "", true); } } .... }


最初の条件では、 sortAscending変数は既に trueですが 、まだ同じ値が割り当てられています。 おそらくこれは間違いであり、 falseを割り当てたかったのでしょう。



別のファイルからの同様の例：





奇妙な関数の戻り値



警告1



V6037ループ内の無条件の「戻り」。 QueryCacheManager.java（128） 



 public <T> T getSingleResultFromCache(QueryKey queryKey, List<View> views) { .... for (Object id : queryResult.getResult()) { return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....)); } .... }


アナライザーは、 forループの最初の繰り返しでreturnステートメントの無条件呼び出しを検出しました。 おそらくこれは間違いであるか、 ifステートメントを使用するようにコードを書き直す必要があります。



警告2



V6014このメソッドが常に1つの同じ値を返すのは奇妙です。 DefaultExceptionHandler.java（40） 



 @Override public boolean handle(ErrorEvent event, App app) { Throwable t = event.getThrowable(); if (t instanceof SocketException || ExceptionUtils.getRootCause(t) instanceof SocketException) { return true; } if (ExceptionUtils.getThrowableList(t).stream() .anyMatch(o -> o.getClass().getName().equals("...."))) { return true; } if (StringUtils.contains(ExceptionUtils.getMessage(t), "....")) { return true; } AppUI ui = AppUI.getCurrent(); if (ui == null) { return true; } if (t != null) { if (app.getConnection().getSession() != null) { showDialog(app, t); } else { showNotification(app, t); } } return true; }


この関数はすべての場合にtrueを返します 。 しかし、最後の行ではfalseの返還をお願いしています 。 おそらく間違いがあります。



同様のコードを持つ疑わしい関数の全リスト：





警告3



V6007式 'needReload'は常にfalseです。 WebAbstractTable.java（2702） 



 protected boolean handleSpecificVariables(Map<String, Object> variables) { boolean needReload = false; if (isUsePresentations() && presentations != null) { Presentations p = getPresentations(); if (p.getCurrent() != null && p.isAutoSave(p.getCurrent()) && needUpdatePresentation(variables)) { Element e = p.getSettings(p.getCurrent()); saveSettings(e); p.setSettings(p.getCurrent(), e); } } return needReload; }


この関数は、値が常にfalseであるneedReload変数を返します。 おそらく、条件の1つで、変数の値を変更するためのコードを追加するのを忘れていました。



警告4



V6062 「isFocused」メソッド内で無限再帰が発生する可能性があります。 GwtAceEditor.java（189）、GwtAceEditor.java（190） 



 public final native void focus() /*-{ this.focus(); }-*/; public final boolean isFocused() { return this.isFocused(); }


アナライザーは、再帰を停止する条件なしで再帰的に呼び出される関数を検出しました。 このファイルには、 ネイティブキーワードでマークされ、コメント化されたコードを含む多くの関数があります。 おそらく、ファイルは現在上書きされており、開発者はすぐにisFocused関数に注意を払うでしょう。



その他の警告



警告1



V6002 switchステートメントは、「操作」列挙のすべての値をカバーしません：ADD。 DesktopAbstractTable.java（665） 



 /** * Operation which caused the datasource change. */ enum Operation { REFRESH, CLEAR, ADD, REMOVE, UPDATE } @Override public void setDatasource(final CollectionDatasource datasource) { .... collectionChangeListener = e -> { switch (e.getOperation()) { case CLEAR: case REFRESH: fieldDatasources.clear(); break; case UPDATE: case REMOVE: for (Object entity : e.getItems()) { fieldDatasources.remove(entity); } break; } }; .... }


switchステートメントはADDの値に対応していません。 レビューされていない唯一のものであるため、間違いかどうかを確認する価値があります。



警告2



V6021変数「source」は使用されません。 DefaultHorizo​​ntalLayoutDropHandler.java（177） 



 @Override protected void handleHTML5Drop(DragAndDropEvent event) { LayoutBoundTransferable transferable = (LayoutBoundTransferable) event .getTransferable(); HorizontalLayoutTargetDetails details = (HorizontalLayoutTargetDetails) event .getTargetDetails(); AbstractOrderedLayout layout = (AbstractOrderedLayout) details .getTarget(); Component source = event.getTransferable().getSourceComponent(); // <= int idx = (details).getOverIndex(); HorizontalDropLocation loc = (details).getDropLocation(); if (loc == HorizontalDropLocation.CENTER || loc == HorizontalDropLocation.RIGHT) { idx++; } Component comp = resolveComponentFromHTML5Drop(event); if (idx >= 0) { layout.addComponent(comp, idx); } else { layout.addComponent(comp); } if (dropAlignment != null) { layout.setComponentAlignment(comp, dropAlignment); } }


ソース変数は宣言されており、コードでは使用されていません。 おそらく、同じタイプの別のcomp変数のように、 レイアウトに ソースを追加するのを忘れていました。



未使用の変数を持つその他の関数：





警告3



V6054クラスは名前で比較しないでください。 MessageTools.java（283） 



 public boolean hasPropertyCaption(MetaProperty property) { Class<?> declaringClass = property.getDeclaringClass(); if (declaringClass == null) return false; String caption = getPropertyCaption(property); int i = caption.indexOf('.'); if (i > 0 && declaringClass.getSimpleName().equals(caption.substring(0, i))) return false; else return true; }


クラスの比較が名前で実行されるときに、アナライザーが状況を検出しました。 仕様によると、JVMクラスはパッケージ内でのみ一意の名前を持つため、このような比較は正しくありません。 これにより、誤った比較と、計画された間違ったコードの実行が発生する可能性があります。



CUBAプラットフォーム開発者レビュー



もちろん、大規模なプロジェクトにはバグがあります。 そのため、PVS-Studioチームがプロジェクトをチェックするという提案に喜んで同意しました。 CUBAリポジトリには、Apache 2ライセンスに基づくいくつかのサードパーティOSSライブラリのフォークが含まれており、このコードにさらに注意を払う必要があるようです。アナライザーはこれらのソースにかなりの数の問題を発見しました。 現在、メインアナライザーとしてSpotBugsを使用していますが、PVS-Studioで見つかった重大な問題は見つかりません。 自分で追加のチェックを書いてみましょう。 PVS-Studioチームの作業に感謝します。



開発者は、警告V6013およびV6054が誤っていることにも注意しました。 コードはそのように意図的に書かれています。 静的アナライザーは、疑わしいコードフラグメントを検出するように設計されており、エラーを検出する確率はすべての検査で異なります。 それでも、ソースコードファイルを変更せずに、アナライザーの警告を大量に抑制する便利なメカニズムを使用すると、このような警告を簡単に処理できます。



別のPVS-Studioチームは、「自分で追加のチェックを書きます」というフレーズを無視して、この写真を残さないでください:)







写真1






おわりに



PVS-Studioは、コード品質ツールを改善するための既存のプロジェクトへの素晴らしい追加となります。 特に、スタッフに数十、数百、数千人の従業員がいる場合。 PVS-Studioは、エラーを見つけるだけでなく、エラーを修正するようにも設計されています。 そして、それは自動コード編集ではなく、信頼できるコード品質管理です。 大企業では、絶対にすべての開発者がさまざまなツールでコードを独立して検証する状況を想像することは不可能であるため、PVS-Studioなどのツールは、通常のプログラマーだけでなく、開発のすべての段階でコードの品質管理が提供される企業に適しています。











英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 PVS-Studioを使用したCUBAプラットフォームのコードの分析


More articles:


All Articles