WildFlyの確認-JavaEEアプリケーションサーバー

WildFly（以前のJBoss Application Server）は、2008年2月にJBossによって作成されたオープンソースのJavaEEアプリケーションサーバーです。 WildFlyプロジェクトの主な目標は、エンタープライズJavaアプリケーションで通常必要とされる一連のツールを提供することです。また、サーバーはエンタープライズアプリケーションの開発に使用されるため、エラーの数とコード内の潜在的な脆弱性を最小限に抑えることが特に重要です。現在、WildFlyは大企業のRed Hatによって開発されており、プロジェクトコードの品質はかなり高いレベルで維持されていますが、アナライザーはプロジェクト内の多くのエラーを検出することができました。



私の名前はドミトリーです。最近、JavaプログラマーとしてPVS-Studioチームに参加しました。ご存知のように、コードアナライザーに慣れるには、実際に試してみるのが一番です。そこで、面白いプロジェクトを選び、チェックして、その結果に基づいて記事を書くことにしました。これはあなたが今読んでいるものです。:)

プロジェクト分析

分析には、GitHubで公開されているWildFlyプロジェクトのソースコードを使用しました。Clocは、空白とコメントを除いて、プロジェクトで60万行のJavaコードをカウントしました。コード内のエラーの検索は、PVS-Studioによって実行されました。PVS-Studioは、C、C ++、C＃、およびJavaで記述されたプログラムのソースコードのバグと潜在的な脆弱性を見つけるためのツールです。IntelliJIDEAバージョン7.09のアナライザープラグインが使用されました。



プロジェクトをチェックした結果、491個のアナライザートリガーのみが受信されました。これは、WildFlyコードの品質が良好であることを示しています。これらのうち、113が高で、146が中です。同時に、ポジティブのまともな部分は診断に当てはまります。

• V6002。switchステートメントは、列挙のすべての値を網羅しているわけではありません。
• V6008。潜在的なnull逆参照。
• V6021。値は「x」変数に割り当てられますが、使用されません。

これらの診断が実際にエラーであるかどうかを理解するのは難しいため、この記事ではこれらの診断のトリガーについては考慮していません。このような警告は、コードの作成者が最もよく理解しています。



次に、私が最も興味深いと思った10個のアナライザートリガーを見ていきます。尋ねる-なぜ10？数がそれのようであるという理由だけで。:)



では、行きましょう。

警告N1は役に立たない条件文です

V6004「then」ステートメントは「else」ステートメントと同等です。WeldPortableExtensionProcessor.java（61）、WeldPortableExtensionProcessor.java（65）。

@Override
public void deploy(DeploymentPhaseContext 
phaseContext) throws DeploymentUnitProcessingException {
    final DeploymentUnit deploymentUnit = phaseContext.getDeploymentUnit();
    // for war modules we require a beans.xml to load portable extensions
    if (PrivateSubDeploymentMarker.isPrivate(deploymentUnit)) {
        if (!WeldDeploymentMarker.isPartOfWeldDeployment(deploymentUnit)) {
          return;
        }
    } else {
        // if any deployments have a beans.xml we need 
        // to load portable extensions
        // even if this one does not.
        if (!WeldDeploymentMarker.isPartOfWeldDeployment(deploymentUnit)) {
           return;
        }
    }
}

内のコードの場合とそれ以外の枝があり、同じ、および条件演算子は、現在の形で意味がありません。開発者がこの方法をこのように書いた理由を考えるのは難しいです。ほとんどの場合、エラーはコピー＆ペーストまたはリファクタリングの結果として発生しました。

N2警告-重複する条件

V6007式 'poolStatsSize> 0'は常に真です。PooledConnectionFactoryStatisticsService.java（85）

@Override
public void start(StartContext context) throws StartException {
  ....
  if (poolStatsSize > 0) {
    if (registration != null) {
      if (poolStatsSize > 0) {
        ....
      }
    }
  }
}

この場合、条件の重複がありました。これはプログラムの結果には影響しませんが、コードの読みやすさを悪化させます。ただし、2番目のチェックに他のより強力な条件が含まれている可能性があります。



WildFlyでトリガーされるこの診断の他の例：

• V6007式 'referralMode == null'は常にfalseです。DirContext.java（93）
• V6007式 'mBeanServer == null'は常にtrueです。WildFlyServerPlatform.java（82）
• V6007式 'result！= Null'は常に真です。Newはnull以外の参照を返します。JarCheck.java（84）
• V6007式 '結果'は常に真です。MultipleAdminObject2Impl.java（147）

警告N3-ヌル参照による参照

V6008「tc」のヌル逆参照。ExternalPooledConnectionFactoryService.java（382）

private void createService(ServiceTarget serviceTarget,
         ServiceContainer container) throws Exception {
   ....
   for (TransportConfiguration tc : connectors) {
     if (tc == null) {
        throw MessagingLogger.ROOT_LOGGER.connectorNotDefined(tc.getName());
     }
   }
   ....
}

彼らは明らかにここを台無しにしました。まず、参照がnullであることを確認してから、この非常にnullの参照に対してgetNameメソッドを呼び出します。これにより、connectorNotDefined（....）から予期される例外ではなく、NullPointerExceptionが発生します。

警告N4-非常に奇妙なコード

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



V6037ループ内の無条件の「スロー」。EJB3Subsystem12Parser.java（81）

protected void readAttributes(final XMLExtendedStreamReader reader)
   throws XMLStreamException {
    for (int i = 0; i < reader.getAttributeCount(); i++) {
      ParseUtils.requireNoNamespaceAttribute(reader, i);
      throw ParseUtils.unexpectedAttribute(reader, i);
    }
}

V6019とV6037の 2つの診断が同時に反応した非常に奇妙な設計。ここでは、ループの1回の反復のみが実行され、その後、無条件のthrowによって終了します。そのため、リーダーに少なくとも1つの属性が含まれている場合、readAttributesメソッドは例外をスローします。このループは、同等の条件に置き換えることができます。

if(reader.getAttributeCount() > 0) {
  throw ParseUtils.unexpectedAttribute(reader, 0);
}

ただし、もう少し深く掘り下げて、requireNoNamespaceAttribute（....）メソッドを確認することができます。

public static void requireNoNamespaceAttribute
 (XMLExtendedStreamReader reader, int index) 
  throws XMLStreamException {
   if (!isNoNamespaceAttribute(reader, index)) {
        throw unexpectedAttribute(reader, index);
   }
}

このメソッドは内部的に同じ例外をスローすることがわかります。ほとんどの場合、readAttributesメソッドは、指定された属性がどの名前名にも属していないこと、および属性が欠落していないことを確認する必要があります。このような構造は、コードのリファクタリングとrequireNoNamespaceAttributeメソッドへの例外のスローの結果として生じたと言いたいです。コミット履歴を見るだけで、このコードがすべて同時に追加されたことがわかります。

警告N5-コンストラクターにパラメーターを渡す

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

public DigestAuthenticationMechanism(final String realmName,
    final String domain, 
    final String mechanismName,
    final IdentityManager identityManager, 
    boolean validateUri) {
       this(Collections.singletonList(DigestAlgorithm.MD5),
            Collections.singletonList(DigestQop.AUTH), 
            realmName, domain, new SimpleNonceManager(), 
            DEFAULT_NAME, identityManager, validateUri);
}

通常、未使用の変数と関数パラメーターは重要なものではありません。通常、これらはリファクタリング後も、将来新しい機能を実装するために追加された後も残ります。しかし、この操作は私にはかなり疑わしいようでした。

public DigestAuthenticationMechanism
  (final List<DigestAlgorithm> supportedAlgorithms, 
   final List<DigestQop> supportedQops,
   final String realmName, 
   final String domain, 
   final NonceManager nonceManager, 
   final String mechanismName, 
   final IdentityManager identityManager,
   boolean validateUri) {....}

クラスの2番目のコンストラクターを見ると、mechanizmName文字列が6番目のパラメーターとして想定されていることがわかります。最初のコンストラクターは、3番目のパラメーターと同じ名前の文字列を受け取り、2番目のコンストラクターを呼び出します。ただし、この文字列は使用されず、代わりに定数が2番目のコンストラクターに渡されます。おそらく、ここでのコードの作成者は、DEFAULT_NAME定数の代わりにmechanismNameをコンストラクターに渡すことを計画していました。

警告N6-行が重複しています

V6033同じキーを持つアイテム 'org.apache.activemq.artemis.core.remoting.impl.netty。

TransportConstants.NIO_REMOTING_THREADS_PROPNAME 'はすでに追加されています。LegacyConnectionFactoryService.java（145）、LegacyConnectionFactoryService.java（139）

private static final Map<String, String> 
PARAM_KEY_MAPPING = new HashMap<>();
....
static {
  PARAM_KEY_MAPPING.put(
    org.apache.activemq.artemis.core.remoting.impl.netty
      .TransportConstants.NIO_REMOTING_THREADS_PROPNAME,
      TransportConstants.NIO_REMOTING_THREADS_PROPNAME);
    ....
  PARAM_KEY_MAPPING.put(
    org.apache.activemq.artemis.core.remoting.impl.netty
      .TransportConstants.NIO_REMOTING_THREADS_PROPNAME,
      TransportConstants.NIO_REMOTING_THREADS_PROPNAME);
    ....
}

アナライザーは、同じキーを持つ2つの値が辞書に追加されたことを報告します。この場合、追加されたキーと値の一致は完全に複製されます。記録された値はTransportConstantsクラスの定数であり、作成者が誤ってコードを複製したか、コピー貼り付け中に値を変更するのを忘れたという2つのオプションのいずれかがあります。ざっと調べたところ、不足しているキーと値が見つからなかったので、最初のシナリオの可能性が高いと思います。

警告N7-変数が失われました

V6046フォーマットが正しくありません。異なる数のフォーマットアイテムが予想されます。欠落している引数：2。TxTestUtil.java（80）

public static void addSynchronization(TransactionManager tm,
          TransactionCheckerSingletonRemote checker) {
  try {
    addSynchronization(tm.getTransaction(), checker);
  } catch (SystemException se) {
     throw new RuntimeException(String
      .format("Can't obtain transaction for transaction manager '%s' "
     + "to enlist add test synchronization '%s'"), se);
  }
}

変数が失われます（必要です）。フォーマットされた文字列には他の2行が代入されるはずでしたが、コードの作成者は明らかにそれらを追加するのを忘れていました。適切な引数なしで文字列をフォーマットすると、開発者が意図したRuntimeExceptionの代わりにIllegalFormatExceptionがスローされます。原則として、IllegalFormatExceptionはRuntimeExceptionを継承しますが、例外に渡されたメッセージは出力で失われ、デバッグ時に正確に何が悪かったのかを理解するのがより困難になります。

警告N8-文字列とオブジェクトの比較

V6058'equals '関数は、互換性のないタイプのオブジェクト（String、ModelNode）を比較します。JaxrsIntegrationProcessor.java（563）

// Send value to RESTEasy only if it's not null, empty string, or the 
// default value.

private boolean isTransmittable(AttributeDefinition attribute,
                                ModelNode modelNode) {
  if (modelNode == null || ModelType
      .UNDEFINED.equals(modelNode.getType())) {
    return false;
  }
  String value = modelNode.asString();
  if ("".equals(value.trim())) {
    return false;
  }
  return !value.equals(attribute.getDefaultValue());        // <=
}

この場合、文字列はオブジェクトと比較され、そのような比較は常にfalseです。つまり、attribute.getDefaultValue（）に等しいmodelNode値がメソッドに渡された場合、メソッドの動作は正しくないことが判明し、コメントにもかかわらず、値は送信に対して有効であると認識されます。 ほとんどの場合、彼らは、attribute.getDefaultValue（）を文字列として表すためにasString（）メソッドを呼び出すのを忘れていました。修正されたバージョンは次のようになります。

return !value.equals(attribute.getDefaultValue().asString());

WildFlyには、V6058診断のもう1つの同様のトリガーがあります。

• V6058'equals '関数は、互換性のないタイプのオブジェクト（String、ObjectTypeAttributeDefinition）を比較します。DataSourceDefinition.java（141）

警告N9-レイトチェック

V6060'dataSourceController '参照は、nullに対して検証される前に使用されました。AbstractDataSourceAdd.java（399）、AbstractDataSourceAdd.java（297）

static void secondRuntimeStep(OperationContext context, ModelNode operation, 
ManagementResourceRegistration datasourceRegistration, 
ModelNode model, boolean isXa) throws OperationFailedException {
  final ServiceController<?> dataSourceController =    
        registry.getService(dataSourceServiceName);
  ....
  dataSourceController.getService()  
  ....
  if (dataSourceController != null) {....}
  ....
}

アナライザーは、オブジェクトがnullであるかどうかがチェックされるずっと前にコードで使用されており、それらの間の距離が102行ものコードであることを検出しました。これは、コードを手動で分析するときに気付くのが非常に困難です。

警告N10-ロックの再確認

V6082安全でないダブルチェックロック。以前に割り当てられたオブジェクトは、別のオブジェクトに置き換えることができます。 JspApplicationContextWrapper.java（74）、JspApplicationContextWrapper.java（72）

private volatile ExpressionFactory factory;
....
@Override
public ExpressionFactory getExpressionFactory() {
  if (factory == null) {
    synchronized (this) {
      if (factory == null) {
        factory = delegate.getExpressionFactory();
        for (ExpressionFactoryWrapper wrapper : wrapperList) {
          factory = wrapper.wrap(factory, servletContext);
        }
      }
    }
  }
  return factory;
}

これは「ダブルチェックロック」パターンを使用し、メソッドが不完全に初期化された変数を返す場合があります。



スレッドAは、値が初期化されていないことに気付いたため、ロックを取得して値の初期化を開始します。この場合、スレッドは、オブジェクトが最後まで初期化される前であっても、オブジェクトをフィールドに書き込むことができます。スレッドBは、オブジェクトが作成されたことを検出して返しますが、スレッドAには、ファクトリですべての作業を行う時間がまだありません。



その結果、計画されたすべてのアクションが実行されていないオブジェクトがメソッドから返される場合があります。

結論

プロジェクトは大企業のRedHatによって開発されており、プロジェクトのコードの品質は高レベルであるにもかかわらず、PVS-Studioを使用して実行された静的分析により、サーバーの動作に何らかの影響を与える可能性のある特定の数のエラーを明らかにすることができました。また、WildFlyはエンタープライズアプリケーションを作成するために設計されているため、これらの間違いは非常に悲しい結果につながる可能性があります。



PVS-Studioをダウンロードして、プロジェクトを確認してください。これを行うには、トライアルライセンスをリクエストするか、無料のユースケースの1つを使用できます。

この記事を英語を話す聴衆と共有したい場合は、翻訳リンクDmitryScherbakovを使用してください。JavaEEアプリケーションサーバーであるWildFlyをチェックしています。