本番のエンタープライズ環境で見たことのある最もevilなコードは何ですか？[クローズド］

質問

これまで企業の本番環境で見た中で、最も邪悪または危険なコード フラグメントは何ですか？ 私は、意図的に悪意のある邪悪なものであると考えられる本番用コードに遭遇したことがないので、他の人が何を見つけたのか、かなり興味があります。

私がこれまでに見た最も危険なコードは、私たちのコアな本番データベース サーバーから 2 つ離れたリンクされたサーバーにあるストアド プロシージャでした。 このストアド プロシージャは、任意の NVARCHAR(8000) パラメータを受け入れ、二重ジャンプ sp_executeSQL コマンドを介してターゲットの本番サーバー上でパラメータを実行しました。 つまり、sp_executeSQLコマンドは、リンクされた2つのサーバーをジャンプさせるために、別のsp_executeSQLコマンドを実行したのです。 ああ、リンクされたサーバーのアカウントは、ターゲットの本番サーバーの sysadmin 権限を持っていました。

どのように解決するのですか?

警告 長い怖い投稿の前に

以前、私が担当したあるアプリケーションについて書いたことがあります。 はこちら と はこちら . 簡単に言うと、私の会社はインドから13万行のゴミを受け継いだのです。そのアプリケーションは C# で書かれており、窓口業務用のアプリで、銀行に行くと必ず窓口の人が使っているのと同じ種類のソフトウェアでした。このアプリは1日に40～50回クラッシュし、単純に動作するコードにリファクタリングすることができなかったのです。私の会社は 12 か月かけてアプリ全体を書き直さなければなりませんでした。

なぜこのアプリケーションは悪なのか？ソース コードを見ただけで、正気の人が狂い、狂人の人が正気になるには十分だったからです。このアプリケーションを書くために使用されたねじれたロジックは、ラブクラフトの悪夢に触発されたとしか思えません。このアプリケーションのユニークな機能は以下の通りです。

• 130,000 行のコードのうち、アプリケーション全体は 5 つのクラス (フォーム ファイルを除く) を含んでいました。これらはすべて、パブリックな静的クラスでした。1 つのクラスは Globals.cs と呼ばれ、アプリケーションの状態全体を保持するために使用される、1000 個、1000 個、1000 個のパブリック静的変数を含んでいました。これら 5 つのクラスには合計で 20,000 行のコードが含まれており、残りのコードはフォームに埋め込まれています。

• プログラマーはどのようにして、クラスなしでこのような大きなアプリケーションを書くことができたのでしょうか? データ オブジェクトを表現するために何を使用したのでしょうか? プログラマーは、ArrayList、HashTables、および DataTables を組み合わせることで、私たちが OOP について学んだ概念の半分を再発明できたことがわかりました。私たちはこれをたくさん目にしました。

  • ハッシュ テーブルの ArrayList
  • 文字列キーとDataRowの値を持つハッシュテーブル
  • データテーブルのArrayList
  • HashTableを含むArrayListを含むDataRows。
  • データロウの配列リスト
  • ArrayListの配列リスト
  • 文字列のキーとHashTableの値を持つHashTable
  • HashTableのArrayListのArrayListのArrayList。
  • ArrayList、HashTables、DataTablesの他のすべての組み合わせが考えられます。

  上記のデータ構造はどれも強く型付けされていないので、リストから取得した謎のオブジェクトを正しい型にキャストする必要があることに注意してください。ArrayList、HashTables、DataTables だけで、どのような複雑なルーブ・ゴールドバーグ的データ構造を作成できるかは、驚くべきことです。

• 上記のオブジェクト モデルをどのように使用するかの例を共有するために、アカウントについて考えてみましょう。元のプログラマは、アカウントの考えられるプロパティごとに個別の HashTable を作成しました: hstAcctExists、hstAcctNeedsOverride、hstAcctFirstName という HashTable です。これらのハッシュテーブルのキーは、すべて「｜」区切りの文字列である。考えられるキーとしては、「123456|DDA」、「24100|SVG」、「100|LNS」などがある。

• アプリケーション全体の状態はグローバル変数から容易にアクセスできたので、プログラマはメソッドにパラメータを渡す必要がないことに気づきました。私は、90% のメソッドが 0 個のパラメータを受け取ったと思います。パラメータを渡した少数のメソッドのうち、すべてのパラメータは、文字列が何を表すかに関係なく、便宜上、文字列として渡されました。

• 副作用のない関数は存在しませんでした。すべてのメソッドは Globals クラスの 1 つ以上の変数を修正しました。たとえば、フォーム検証メソッドの 1 つには、Globals.lngAcctNum が格納されているアカウントに対してローンの過払いと不足を計算するという、謎の副作用がありました。

• たくさんのフォームがありましたが、それらを支配する 1 つのフォームがありました: なんと 20,000 行ものコードを含む frmMain.cs です。frmMainは何をしたのでしょうか？すべてだ。勘定科目を調べたり、領収書を印刷したり、現金を払い出したり、なんでもできる。

  時々、他のフォームがfrmMainのメソッドを呼び出す必要がありました。そのコードをフォームから別のクラスにファクタリングするのではなく、コードを直接呼び出すのはいかがなものでしょうか。

  ((frmMain)this.MDIParent).UpdateStatusBar(hstValues);
  
  

• アカウントを調べるために、プログラマは次のようなことをしました。

  bool blnAccountExists =
      new frmAccounts().GetAccountInfo().blnAccountExists
  
  

  ビジネスロジックを実行するために目に見えないフォームを作成することは既に悪いことですが、フォームがどのアカウントを検索するのかどうやって知ったと思いますか？それは簡単です。フォームはGlobals.lngAcctNumとGlobals.strAcctTypeにアクセスすることができたからです。(ハンガリー語の記法が嫌いな人はいないでしょう?)

• Code-reuse は ctrl-c、ctrl-v の同義語でした。200行のメソッドを20のフォームにコピー＆ペーストしているのを発見しました。

• アプリケーションには、私がスレッド アンド タイマー モデルと呼びたいような、奇妙なスレッド モデルがありました。一旦タイマーが始まると、スレッドが何らかの魔法のブール値を設定したかどうかを確認し、スレッドを中止します。結果として生じる ThreadAbortException は飲み込まれました。

  このパターンは一度しか見ないと思うかもしれませんが、私は少なくとも 10 の異なる場所でそれを発見しました。

• スレッドといえば、キーワード "lock"はアプリケーションに一度も現れませんでした。スレッドはロックを取ることなく、自由にグローバルな状態を操作していました。

• アプリケーションのすべてのメソッドは、try/catch ブロックを含んでいました。すべての例外はログに記録され、飲み込まれました。

• 文字列の切り替えが同じくらい簡単なのに、誰が列挙型の切り替えをする必要があるのでしょうか!

• ある天才は、複数のフォームコントロールを同じイベントハンドラにフックすることができることを発見しました。プログラマーはこれをどのように処理したのでしょうか?

  private void OperationButton_Click(object sender, EventArgs e)
  {
      Button btn = (Button)sender;
      if (blnModeIsAddMc)
      {
          AddMcOperationKeyPress(btn);
      }
      else
      {
          string strToBeAppendedLater = string.Empty;
          if (btn.Name != "btnBS")
          {
              UpdateText();
          }
          if (txtEdit.Text.Trim() != "Error")
          {
              SaveFormState();
          }
          switch (btn.Name)
          {
              case "btnC":
                  ResetValues();
                  break;
              case "btnCE":
                  txtEdit.Text = "0";
                  break;
              case "btnBS":
                  if (!blnStartedNew)
                  {
                      string EditText = txtEdit.Text.Substring(0, txtEdit.Text.Length - 1);
                      DisplayValue((EditText == string.Empty) ? "0" : EditText);
                  }
                  break;
              case "btnPercent":
                  blnAfterOp = true;
                  if (GetValueDecimal(txtEdit.Text, out decCurrValue))
                  {
                      AddToTape(GetValueString(decCurrValue), (string)btn.Text, true, false);
                      decCurrValue = decResultValue * decCurrValue / intFormatFactor;
                      DisplayValue(GetValueString(decCurrValue));
                      AddToTape(GetValueString(decCurrValue), string.Empty, true, false);
                      strToBeAppendedLater = GetValueString(decResultValue).PadLeft(20)
                                                  + strOpPressed.PadRight(3);
                      if (arrLstTapeHist.Count == 0)
                      {
                          arrLstTapeHist.Add(strToBeAppendedLater);
                      }
                      blnEqualOccurred = false;
                      blnStartedNew = true;
                  }
                  break;
              case "btnAdd":
              case "btnSubtract":
              case "btnMultiply":
              case "btnDivide":
                  blnAfterOp = true;
                  if (txtEdit.Text.Trim() == "Error")
                  {
                      btnC.PerformClick();
                      return;
                  }
                  if (blnNumPressed || blnEqualOccurred)
                  {
                      if (GetValueDecimal(txtEdit.Text, out decCurrValue))
                      {
                          if (Operation())
                          {
                              AddToTape(GetValueString(decCurrValue), (string)btn.Text, true, true);
                              DisplayValue(GetValueString(decResultValue));
                          }
                          else
                          {
                              AddToTape(GetValueString(decCurrValue), (string)btn.Text, true, true);
                              DisplayValue("Error");
                          }
                          strOpPressed = btn.Text;
                          blnEqualOccurred = false;
                          blnNumPressed = false;
                      }
                  }
                  else
                  {
                      strOpPressed = btn.Text;
                      AddToTape(GetValueString(0), (string)btn.Text, false, false);
                  }
                  if (txtEdit.Text.Trim() == "Error")
                  {
                      AddToTape("Error", string.Empty, true, true);
                      btnC.PerformClick();
                      txtEdit.Text = "Error";
                  }
                  break;
              case "btnEqual":
                  blnAfterOp = false;
                  if (strOpPressed != string.Empty || strPrevOp != string.Empty)
                  {
                      if (GetValueDecimal(txtEdit.Text, out decCurrValue))
                      {
                          if (OperationEqual())
                          {
                              DisplayValue(GetValueString(decResultValue));
                          }
                          else
                          {
                              DisplayValue("Error");
                          }
                          if (!blnEqualOccurred)
                          {
                              strPrevOp = strOpPressed;
                              decHistValue = decCurrValue;
                              blnNumPressed = false;
                              blnEqualOccurred = true;
                          }
                          strOpPressed = string.Empty;
                      }
                  }
                  break;
              case "btnSign":
                  GetValueDecimal(txtEdit.Text, out decCurrValue);
                  DisplayValue(GetValueString(-1 * decCurrValue));
                  break;
          }
      }
  }
  
  

• 同じ天才が、栄光の三項演算子も発見しました。以下はコードサンプルです。

  frmTranHist.cs [line 812]:

  strDrCr = chkCredits.Checked && chkDebits.Checked ? string.Empty
                      : chkDebits.Checked ? "D"
                          : chkCredits.Checked ? "C"
                              : "N";
  
  

  frmTellTransHist.cs [line 961]:

  if (strDefaultVals == strNowVals && (dsTranHist == null ? true : dsTranHist.Tables.Count == 0 ? true : dsTranHist.Tables[0].Rows.Count == 0 ? true : false))
  
  

  frmMain.TellCash.cs [line 727]:

  if (Validations(parPostMode == "ADD" ? true : false))
  
  

• StringBuilderの典型的な誤用を示すコードスニペットを示します。プログラマがループ内で文字列を連結し、その結果の文字列をStringBuilderに追加していることに注目してください。

  private string CreateGridString()
  {
      string strTemp = string.Empty;
      StringBuilder strBuild = new StringBuilder();
      foreach (DataGridViewRow dgrRow in dgvAcctHist.Rows)
      {
          strTemp = ((DataRowView)dgrRow.DataBoundItem)["Hst_chknum"].ToString().PadLeft(8, ' ');
          strTemp += "  ";
          strTemp += Convert.ToDateTime(((DataRowView)dgrRow.DataBoundItem)["Hst_trandt"]).ToString("MM/dd/yyyy");
          strTemp += "  ";
          strTemp += ((DataRowView)dgrRow.DataBoundItem)["Hst_DrAmount"].ToString().PadLeft(15, ' ');
          strTemp += "  ";
          strTemp += ((DataRowView)dgrRow.DataBoundItem)["Hst_CrAmount"].ToString().PadLeft(15, ' ');
          strTemp += "  ";
          strTemp += ((DataRowView)dgrRow.DataBoundItem)["Hst_trancd"].ToString().PadLeft(4, ' ');
          strTemp += "  ";
          strTemp += GetDescriptionString(((DataRowView)dgrRow.DataBoundItem)["Hst_desc"].ToString(), 30, 62);
          strBuild.AppendLine(strTemp);
      }
      strCreateGridString = strBuild.ToString();
      return strCreateGridString;//strBuild.ToString();
  }
  
  

• テーブルには主キー、インデックス、外部キー制約は存在せず、ほぼすべてのフィールドは varchar(50) 型であり、100%のフィールドが null 可能でした。興味深いことに、ビット フィールドはブーリアン データを格納するために使用されませんでした。代わりに char(1) フィールドが使用され、文字 'Y' と 'N' がそれぞれ true と false を表すために使用されました。

  • データベースといえば、ストアドプロシージャの代表的な例です。

    ALTER PROCEDURE [dbo].[Get_TransHist]
     ( 
          @TellerID   int = null,
          @CashDrawer int = null,
          @AcctNum    bigint = null,
          @StartDate  datetime = null,
          @EndDate    datetime = null,
          @StartTranAmt     decimal(18,2) = null,
          @EndTranAmt decimal(18,2) = null,
          @TranCode   int = null,
          @TranType   int = null
     )
    AS 
          declare @WhereCond Varchar(1000)
          declare @strQuery Varchar(2000)
          Set @WhereCond = ' '
          Set @strQuery = ' '
          If not @TellerID is null
                Set @WhereCond = @WhereCond + ' AND TT.TellerID = ' + Cast(@TellerID as varchar)
          If not @CashDrawer is null
                Set @WhereCond = @WhereCond + ' AND TT.CDId = ' + Cast(@CashDrawer as varchar)
          If not @AcctNum is null
                Set @WhereCond = @WhereCond + ' AND TT.AcctNbr = ' + Cast(@AcctNum as varchar)
          If not @StartDate is null
                Set @WhereCond = @WhereCond + ' AND Convert(varchar,TT.PostDate,121) >= ''' + Convert(varchar,@StartDate,121) + ''''
          If not @EndDate is null
                Set @WhereCond = @WhereCond + ' AND Convert(varchar,TT.PostDate,121) <= ''' + Convert(varchar,@EndDate,121) + ''''
          If not @TranCode is null
                Set @WhereCond = @WhereCond + ' AND TT.TranCode = ' + Cast(@TranCode as varchar)
          If not @EndTranAmt is null
                Set @WhereCond = @WhereCond + ' AND TT.TranAmt <= ' + Cast(@EndTranAmt as varchar)
          If not @StartTranAmt is null
                Set @WhereCond = @WhereCond + ' AND TT.TranAmt >= ' + Cast(@StartTranAmt  as varchar)
          If not (@TranType is null or @TranType = -1)
                Set @WhereCond = @WhereCond + ' AND TT.DocType = ' + Cast(@TranType as varchar)
          --Get the Teller Transaction Records according to the filters
          Set @strQuery = 'SELECT 
                TT.TranAmt as [Transaction Amount], 
                TT.TranCode as [Transaction Code],
                RTrim(LTrim(TT.TranDesc)) as [Transaction Description],
                TT.AcctNbr as [Account Number],
                TT.TranID as [Transaction Number],
                Convert(varchar,TT.ActivityDateTime,101) as [Activity Date],
                Convert(varchar,TT.EffDate,101) as [Effective Date],
                Convert(varchar,TT.PostDate,101) as [Post Date],
                Convert(varchar,TT.ActivityDateTime,108) as [Time],
                TT.BatchID,
                TT.ItemID,
                isnull(TT.DocumentID, 0) as DocumentID,
                TT.TellerName,
                TT.CDId,
                TT.ChkNbr,
                RTrim(LTrim(DT.DocTypeDescr)) as DocTypeDescr,
                (CASE WHEN TT.TranMode = ''F'' THEN ''Offline'' ELSE ''Online'' END) TranMode,
                DispensedYN
          FROM TellerTrans TT WITH (NOLOCK)
          LEFT OUTER JOIN DocumentTypes DT WITH (NOLOCK) on DocType = DocumentType
          WHERE IsNull(TT.DeletedYN, 0) = 0 ' + @WhereCond + ' Order By BatchId, TranID, ItemID'    
          Exec (@strQuery)
    
    

とはいえ、この 130,000 行のアプリケーションの最大の問題は、ユニットテストがないことです。

そう、私はこの話をTheDailyWTFに送り、そして仕事を辞めたのです。