◀Unicode版開発トップへ
  • 2382 セキュアなsnprintf関数。
    • 2383 Re:セキュアなsnprintf関数。
      • 2384 Re2:セキュアなsnprintf関数。
        • 2385 Re3:セキュアなsnprintf関数。
          • 2386 Re4:セキュアなsnprintf関数。
            • 2387 Re5:セキュアなsnprintf関数。
            • 2388 Re5:セキュアなsnprintf関数。
              • 2390 Re6:セキュアなsnprintf関数。
      • 2393 Re2:補足

  • [2382] セキュアなsnprintf関数。 ばぼ 2016年11月27日 00:12

    この１年間、荒らしのごとく掲示板に常駐しておりますが、
    そろそろ何かリアクションが欲しいです。
    sourceforgeのほうにパッチを送り続けるにしても、
    現状の積滞パッチ件数95件、という状況から見て、
    反応があるまで待っていられる自信がありません。

    残件95件だと週1で消化したとしても2年です。
    ここ1年の実績から考えると、週1なんて夢のようなペースですよね。

    このままのらりくらりと、
    見付けた不審点を「気ままに報告～」を続けていると
    sakura editorのソフトウェアとしての信用に関わる気がします。
    わたしとしては、それはすごく不本意なんです。

    プロから見れば全然大したことない不具合であっても、
    公開されたインターネットで指摘され、
    しかも、長い間「未解決」のまま放置されている。。。
    いまのsakura editorってそうなりつつあります。

    コミッタ足りないなら手伝います。
    ガチヲタ１匹、プロジェクトに参加させてください。


    こないだsoureceforgeに書いた「auto系使えね」発言の責任をとるべく、
    「使えるように整備してやろうじゃないか！」と
    定義関数の総チェックに乗り出したところ、変なもの見付けてしまいました。

    int auto_snprintf_s(WCHAR* buf, size_t count, const WCHAR* format, ...);

    わたしが実装するとすれば↓な定義を作ると思います。
    int auto_snprintf (WCHAR* buf, size_t count, const WCHAR* format, ...);
    int auto_snprintf_s(WCHAR* buf, size_t nBufSize, size_t count, const WCHAR* format, ...);
    template<size_t _BufSize>
    int auto_snprintf_s(WCHAR (&buf)[_BufSize], size_t count, const WCHAR* format, ...);

    指摘されれば「え、うそ。」みたいに思いますよね？
    セキュア版関数って、一体何がセキュアなのか。

    中の人向けなので詳しい内容は書きません。
    是非、ご検討ください。

    • [2383] Re:セキュアなsnprintf関数。 LR4 2016年11月27日 00:56

      ▼ ばぼさん
      > この１年間、荒らしのごとく掲示板に常駐しておりますが、
      > そろそろ何かリアクションが欲しいです。

      え？、もう一年も経ったかしら
      ていうか、一方的なゴリ押ししかしてないのに、
      三年程度も待てず、一年足らずで物申すとか、あり得ない

      > 指摘されれば「え、うそ。」みたいに思いますよね？

      水爆の父（Edward Teller）にでもなりたいかな？
      それ、求めすぎてて、たとえ職人といえど異常ですからb
      ロジックにだけ特化した気狂い

      そう言われた覚えが今までに一度も無い、ってなら、どんだけ周囲が気遣ってくれてるんだか、自覚が必要

      他に大事なもの、たくさんあるんだけどなぁ…
      せめて「まともなAero Snap対応」とか「Per-Monitor DPI対応」くらい成し遂げてくれれば、少しは見込みもありそうだけれど

      > ガチヲタ１匹、プロジェクトに参加させてください。

      おや、自信満々ですなぁ、誘われもせず「参加させろ」とは
      ※おいら的には「ください」は「させろ」と同意義なのねん

      安易にコミッタ志願とか、無いわ
      http://sakura-editor.sourceforge.net/cgi-bin/cyclamen/cyclamen.cgi?log=unicode&tree=r2120

      単なる歯車としてであれば、彼の人の参加も有意義なのかもしれないけれど

      さすがに、この程度の挑発に乗る程度の人材なら切ってしまったほうがいいと思われ
      だって、その程度なら本人が病になるほどに苦労するだけだから

      口が過ぎて申し訳ありません、気の毒な発言をしてしまいました。
      今、この場で土下座しときます

      • [2384] Re2:セキュアなsnprintf関数。 ばぼ 2016年11月27日 18:06

        ▼ LR4さん
        > 口が過ぎて申し訳ありません、気の毒な発言をしてしまいました。
        > 今、この場で土下座しときます

        いえ、問題ありません。
        こちらこそ、あおるように書いてしまいました。
        大変失礼しました。深くお詫びします。

        私は現状のsakura editorの機能に満足しているので、
        目新しい機能の追加よりも、機能の安定化に興味があります。
        派手な機能追加の提案を行うことは、今後もありません。
        機能追加の実装実績が見込みなのであれば、もうゼロです。


        例示は「お題」ですかね。

        Aero Snapは、邪魔になる場合があるのでオフにしてます。
        使わない人間が手をだしちゃいかん機能ってありますよね。

        Per Monitor DPIは多少興味があります。
        作ったらFeature Requestsに入れます。
        時期はクリスマス頃になると思います。
        要らんかもしれませんが。


        > おや、自信満々ですなぁ、誘われもせず「参加させろ」とは
        > ※おいら的には「ください」は「させろ」と同意義なのねん

        否定しません。

        禍根の残らないように書いてしまうと
        文脈的にはもっと酷いこと言ってるんです。
        「見てられないから代われ」と言ってます。
        実際にそこまでのことは思ってなかったんですが、
        「参加させろ」の意図で書いたのは事実で、
        どう考えても失礼な発言で、深く反省しております。

        ただ、そのことと現状のままでよいかは別の問題で、
        今後も自分なりに改善の努力を続けていきたいと考えています。

        • [2385] Re3:セキュアなsnprintf関数。 もか 2016年11月27日 22:38

          >中の人向けなので詳しい内容は書きません
          半分中の人です。中の人向けだからこそ書いてほしいです。

          他人・自分の書いたコードの中には「許容可能」「ぎりぎり許容可能」「許容不可能」があります。
          そのへんの閾値・種類は人によって異なるわけです。
          例えば、new[]をdelete([]なし)で解放するとか、私は「許容不可能」に分類しています。
          いっぽうで、私はauto_strlen,_tcslen,wcslen,lstrlenの混用をしょうがないので許容しています。
          コーディングスタイルは、いまさら言っても始まらないので諦めています。

          衝突の原因になるので、あまり言いたくはないですが
          ぼぼさんに感じるのは、私とは信仰している宗教が違うみたいだ。
          という点です。
          宗教が違っても、お互い別の国に住んでる場合には問題ないのだけど、
          同じ家に住んで、一緒の居間で過ごすとなると、双方に相応の努力が必要になってきます。

          とりあえずぼぼさんは、セキュリティ対策っていうヤバめなタイトルのパッチを
          ・あるなら「本当にやばいと思う部分」脆弱性の修正(これは公開せずコミッタに直接送ってもいいです)
          ・予防措置的なセキュリティ対策
          ・inlineとか10マイクロ秒早くなるとかの、趣味的なもの
          ・初期化ミスとかのバグ類
          みたいに意味のある単位に分離して、書き直した方がいいです。
          でないと、でかいごちゃ混ぜの物を置いてかれても真面目に相手をしてもらえないです。

          auto_snprintf_sについてはBufSize, countの2引数のそれぞれの意味と、
          正しい想定した使い方を説明しないと「ただ見せびらかしてるだけ」です。
          もちろんcount1引数ではダメである論理的な説明も必要です。
          しかもこの前のauto_*はstrlenに関する話であってsprintfは無関係のはずです。
          なお私は、10マイクロ秒、30バイト節約できるみたいな話に興味はありません。
          でもコードの間違いが減る、コード変更に対する耐性が高くなる、簡素(分かりやすく)に書けるなどには興味があります。
          ちなみに「え、うそ。」みたいには別に思いません。そう言う宗教もあるな、と思うだけです。

          バッファ長を2引数にしたところで
          TCHAR szRealBuf[100];
          TCHAR* szBuf = szRealBuf;
          auto_snprintf_s(szBuf, sizeof(szBuf), sizeof(szBuf), _T("%d"), linenum);
          みたいに他人に書かれるのを防ぐ手立てがないんですよね。
          しかも、後で変数宣言の方が書き変わったというパターンが多いので、見逃す可能性を否定できません。
          結局最後は使う人だもの。正しい使い方を説明できなきゃだめだわ。
          もちろん自分で説明する必要はないです、必要十分なURLを提示してくれれば。
          追記：この例はよくないね。_countofを普通使うからエラーになる。(追記ここまで)

          この二つは何がどう違うのか説明可能ですか。
          void my_functuin1(wchar* out, size_t size){
          auto_snprintf_s(out, size, size, L"%d", g_linenum);
          }
          void my_functuin2(wchar* out, size_t size){
          auto_snprintf(out, size, L"%d", g_linenum);
          }

          • [2386] Re4:セキュアなsnprintf関数。 ばぼ 2016年11月30日 01:15

            > この二つは何がどう違うのか説明可能ですか。
            > void my_functuin1(wchar* out, size_t size){
            > auto_snprintf_s(out, size, size, L"%d", g_linenum);
            > }
            > void my_functuin2(wchar* out, size_t size){
            > auto_snprintf(out, size, L"%d", g_linenum);
            > }

            ・1はoutにsize - 1文字書き込んで'\0'を付加します(溢れません)。
            　出力がsizeを越えた場合、出力はsize-1文字に切り詰められます(溢れません)。
            　出力がsizeを越えた場合、バッファサイズが足りない警告(戻り値!=0)が出ます。
            ・2はoutにsize文字書き込んで'\0'を付加します(出力=sizeで溢れます)。
            　出力がsizeを越えた場合、出力はsize-1文字に切り詰められます(溢れません)。


            いちおうちゃんと説明しましょう。

            何が問題なのかを知るためには
            sprintfシリーズの関数について、
            発生した経緯から語るのがよさそうです。

            まず、sprintf関数について。
            40年くらい前、C言語が発生すると同時に生まれました。

            　sprintf(out, format, ...)
            　outに...の内容をformatの書式で出力する。

            任意のデータを書式出力するprintf系関数の１つです。
            出力先がメモリなのでI/O処理が要らず高速です。
            確保したメモリの領域外に出力があふれてしまう障害、
            バッファオーバーフローの危険が古くから指摘される関数です。

            つぎに、snprintf関数。
            15年くらい前にC言語の規格が改定されるときにできました。
            sprintfが抱えるオーバーフロー問題への対策版です。

            　snprintf(out, count, format, ...)
            　countで最大文字数を指定し、出力を制限できる。

            count文字出力すると末尾に'\0'を付加して終了します。
            出力数がcount+1になるのが微妙ですが、
            これはC言語が規格として提供するセキュア版sprintfです。

            最後にマイクロソフトのセキュア版関数について。
            10年くらい前、vc++2005が出たときにできました。
            ほとんどすべてのメモリ書込みを行う関数に_s付きバージョンができました。

            　sprintf_s(out, size, format, ...)

            size - 1文字出力すると末尾に'\0'を付加して終了します。
            最大出力数はsizeになります。
            sprintf_s関数を使えば、sprintfの抱える問題をカバーできます。


            じゃあ、snprintf_s関数は？

            　snprintf_s(out, size, count, format, ...)

            ・・・ぶっちゃけるとわたしにもよく分りません。

            countには _TRUNCATE という特別な値を指定できるのですが、
            _TRUNCATEを指定した場合の動作はsprintf_sと等価です。
            size - 1文字を上限に、できるだけたくさん出力する、という動きになります。
            なんで存在するのかよく分らない関数です。

            いま、snprintf_sが定義されていて、
            snprintfがない状態になっています。
            当然ですが、sprintf_sはちゃんとあります。

            必要性が微妙な関数が間違った定義で定義されていて、
            本来の使い方をできない状態になっています。
            （sprintf_sを使うべき箇所に誤ったsnprintf_s利用）

            LR4氏の言うように「もっと大事なことがある」と思います。
            ただ、他の部分の共通関数の品質は高いので、放置したままにしとくのももったいないなーとも思います。

            • [2387] Re5:セキュアなsnprintf関数。 もか 2016年11月30日 16:30

              >sprintf_s(out, size, format, ...)
              >size - 1文字出力すると末尾に'\0'を付加して終了します。

              >countには _TRUNCATE という特別な値を指定できるのですが、
              >_TRUNCATEを指定した場合の動作はsprintf_sと等価です。
              言っている内容がいまいち理解できないのは、たぶんこの前提が間違ってるからだと思いました。

              char test[4];
              int ret = _snprintf_s(test, 4, _TRUNCATE, "%s", "1234567890");
              >ret=-1 test="123"
              int ret = _snprintf_s(test, 4, 3, "%s", "1234567890");
              >ret=-1 test="123"
              int ret = _snprintf_s(test, 4, 4, "%s", "1234567890");
              >実行時エラー
              int ret = sprintf_s(test, 4, "%s", "1234567890");
              >実行時エラー
              こうなるみたいです。
              あとsprintf_sはVC2005では存在していなくて「_sprintf_s」ですよね。

              >snprintfがない状態になっています。
              参考 https://sourceforge.net/p/sakura-editor/patchunicode/82/
              >どう倒しても紛らわしいので、
              >・VCの _snprintf に対応する auto関数は提供しない。
              >・今の auto_snprintf は auto_snprintf_s に変名。
              と書かれています。

              auto_snprintf_s→サクラでは_TRUNCATE前提でバッファ切り詰めで使う&-1
              auto_sprintf_s→あふれたら異常終了
              auto_sprintf→あふれないことが保証されたときに使ってもいい
              どれも、引数のバッファ長は\0分を含める実装となっていると。

            • [2388] Re5:セキュアなsnprintf関数。 もか 2016年11月30日 16:31

              それ以前の問題として、サクラの定義をよく見てみると、数か所間違いがあるようにみえます。
              defineのほうはrev:3680以降からです。
              こうじゃないでしょうか。

              Index: sakura_core/util/string_ex.h
              ===================================================================
              --- sakura_core/util/string_ex.h (リビジョン 4157)
              +++ sakura_core/util/string_ex.h (作業コピー)
              @@ -253,9 +253,9 @@

              //印字系
              #if defined(_MSC_VER) && _MSC_VER>=1400
              -#define auto_snprintf_s(buf, count, format, ...) tchar_sprintf_s((buf), count, (format), __VA_ARGS__)
              +#define auto_snprintf_s(buf, count, format, ...) tchar_snprintf_s((buf), count, (format), __VA_ARGS__)
              #define auto_sprintf(buf, format, ...) tchar_sprintf((buf), (format), __VA_ARGS__)
              -#define auto_sprintf_s(buf, nBufCount, format, ...) tchar_snprintf_s((buf), nBufCount, (format), __VA_ARGS__)
              +#define auto_sprintf_s(buf, nBufCount, format, ...) tchar_sprintf_s((buf), nBufCount, (format), __VA_ARGS__)
              #else
              (すみません省略します)
              Index: sakura_core/util/tchar_printf.cpp
              ===================================================================
              --- sakura_core/util/tchar_printf.cpp (リビジョン 4157)
              +++ sakura_core/util/tchar_printf.cpp (作業コピー)
              @@ -359,6 +359,7 @@


              // sprintf_sラップ
              +// ※nBufCountをはみ出す場合は異常終了します。
              //
              // (実装について)
              // 内容が同じなので、templateでも良かったのですが、
              @@ -418,7 +419,7 @@
              {
              va_list v;
              va_start(v,format);
              - int ret=tchar_vsprintf_s(buf,count,format,v);
              + int ret=tchar_vsnprintf_s(buf,count,format,v);
              va_end(v);
              return ret;
              }
              @@ -426,7 +427,7 @@
              {
              va_list v;
              va_start(v,format);
              - int ret=tchar_vsprintf_s(buf,count,format,v);
              + int ret=tchar_vsnprintf_s(buf,count,format,v);
              va_end(v);
              return ret;
              }

              • [2390] Re6:セキュアなsnprintf関数。 ばぼ 2016年12月03日 00:36

                ▼ もかさん
                > それ以前の問題として、サクラの定義をよく見てみると、数か所間違いがあるようにみえます。
                > defineのほうはrev:3680以降からです。

                やっと言ってる意味が分かりました。
                わたしの修正コードでなくて、
                現在のコードに対する修正提案なんですね。


                (tchar_snprintf_sの実装部分)
                > - int ret=tchar_vsprintf_s(buf,count,format,v);
                > + int ret=tchar_vsnprintf_s(buf,count,format,v);

                コア実装がいまのままなら同じことなんで
                間違っていても機能的には影響ないと思います。

                機能的に影響がなければ直さない、
                それが乙女のポリシー。

                と言われたらもう何も言えねっす。


                ちなみに、tchar_snprintf_sは直呼びじゃなく
                auto_snprintf_sを経由するのが正道と思われます。

                string_ex.h 260行目
                inline int auto_snprintf_s(ACHAR* buf, size_t count, const ACHAR* format, ...) { va_list v; va_start(v,format); int ret=tchar_vsnprintf_s(buf,count,format,v); va_end(v); return ret; }
                inline int auto_snprintf_s(WCHAR* buf, size_t count, const WCHAR* format, ...) { va_list v; va_start(v,format); int ret=tchar_vsnprintf_s(buf,count,format,v); va_end(v); return ret; }
                inline int auto_sprintf(ACHAR* buf, const ACHAR* format, ...) { va_list v; va_start(v,format); int ret=tchar_vsprintf(buf,format,v); va_end(v); return ret; }
                inline int auto_sprintf(WCHAR* buf, const WCHAR* format, ...) { va_list v; va_start(v,format); int ret=tchar_vsprintf(buf,format,v); va_end(v); return ret; }
                inline int auto_sprintf_s(ACHAR* buf, size_t nBufCount, const ACHAR* format, ...){ va_list v; va_start(v,format); int ret=tchar_vsprintf_s(buf,nBufCount,format,v); va_end(v); return ret; }
                inline int auto_sprintf_s(WCHAR* buf, size_t nBufCount, const WCHAR* format, ...){ va_list v; va_start(v,format); int ret=tchar_vsprintf_s(buf,nBufCount,format,v); va_end(v); return ret; }

                こっちは合ってますな。

                内部実装のva_arg版関数を呼び出すだけの代物なので、
                呼び出し口で１か所対応すれば目的を果たせるはずです。
                邪道を利用してる箇所は非常に少なかったはずなので、
                呼出し側を修正して関数定義ごと削除するのが適切かと思います。

                関数定義ごと削除してしまえば
                誤って利用してしまうことの再発防止にもなりますし。

                また、全く同じ意味の実体が別定義で存在するので
                機能を損なうこともありません。


                まぁ、機能的影響はゼロなので、
                どうでもいいっちゃどうでもいい話。

      • [2393] Re2:補足 SNK 2016年12月23日 06:54

        ちょっと、公共の場にふさわしくない発言があったように思います。
        いくら匿名のやりとりでも、いや、匿名だからこそ相手のことを思いやって書き込みするべきでしょう。
        もうすこし時間をおいて頭を冷やしてから返答ボタンを押すべきだったと感じます。

        参考:
        http://www.iajapan.org/rule/rule4general/
        インターネットにおけるルール&マナー「大人版」5.15　誹謗・中傷しない

        それはさておき、プロジェクト方針に関する部外者からの意見として、続きを[管理]掲示板に書きます。