2016/11/27 (日) 00:12:08 ばぼ 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; rv:11.0) like Gecko
[2382] セキュアなsnprintf関数。
この1年間、荒らしのごとく掲示板に常駐しておりますが、
そろそろ何かリアクションが欲しいです。
sourceforgeのほうにパッチを送り続けるにしても、
現状の積滞パッチ件数95件、という状況から見て、
反応があるまで待っていられる自信がありません。

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

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

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

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


こないだ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, ...);

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

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

2016/11/27 (日) 00:56:27 LR4 返信 削除
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
[2383] Re:セキュアなsnprintf関数。
▼ ばぼさん
> この1年間、荒らしのごとく掲示板に常駐しておりますが、
> そろそろ何かリアクションが欲しいです。


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

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

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

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

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

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

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

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

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

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

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

2016/11/27 (日) 18:06:33 ばぼ 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; rv:11.0) like Gecko
[2384] Re2:セキュアなsnprintf関数。
▼ LR4さん
> 口が過ぎて申し訳ありません、気の毒な発言をしてしまいました。
> 今、この場で土下座しときます


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

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


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

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

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


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


否定しません。

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

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

2016/11/27 (日) 22:38:24 もか 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
[2385] Re3:セキュアなsnprintf関数。
>中の人向けなので詳しい内容は書きません
半分中の人です。中の人向けだからこそ書いてほしいです。

他人・自分の書いたコードの中には「許容可能」「ぎりぎり許容可能」「許容不可能」があります。
そのへんの閾値・種類は人によって異なるわけです。
例えば、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);
}

2016/11/30 (水) 01:15:39 ばぼ 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; rv:11.0) like Gecko
[2386] Re4:セキュアなsnprintf関数。
> この二つは何がどう違うのか説明可能ですか。
> 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系関数の1つです。
出力先がメモリなので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氏の言うように「もっと大事なことがある」と思います。
ただ、他の部分の共通関数の品質は高いので、放置したままにしとくのももったいないなーとも思います。

2016/11/30 (水) 16:30:13 もか 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
[2387] Re5:セキュアなsnprintf関数。
>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分を含める実装となっていると。

2016/12/2 (金) 23:46:16 ばぼ 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; rv:11.0) like Gecko
[2389] Re6:セキュアなsnprintf関数。
やっちゃいましたね、わたし。。。
嘘つきました。
すんませんでした。

▼ もかさん
> >countには _TRUNCATE という特別な値を指定できるのですが、
> >_TRUNCATEを指定した場合の動作はsprintf_sと等価です。

> 言っている内容がいまいち理解できないのは、たぶんこの前提が間違ってるからだと思いました。


完全に嘘ですね。
確認に auto_sprintf_s を使ってどうする?という話です。

ただ、指摘内容を分かってもらえないのは違う理由です。
もかさんの言葉を借りると宗教が異なるからだと思います。

面倒くさがりなのでバグや混乱のもとになるものは
可能な限り「一律排除」したいと考えています。
過激な保守派ですね。(…いつの間に俺は右翼になった・・・

冗談はさておき。

具体的なコードで話したほうが話やすそうに思ったので、
サンプルコードを書きました。
sprintfだけの話じゃなくなってややこしいんですが、
自分の考えるsprintf関数の正しい使い方です。


//(A) 出力文字数を設計できる場合はsprintfを使ってもいい
char szKey[9];
uint8_t index = 255; //←本当は引数
_sprintf_s(szKey, "KEY[%03d]", index);


//(B) 出力文字数が揺れる場合は、
//出力文字数を計算して適切なサイズのバッファを
//確保したうえでsnprintfを使う

char *title = "sakura"; //←本当は引数
char *ext = ".ini"; //←本当は引数
char *format = "%s_bak%s"; //←本当は引数
size_t nBufLen = _scprintf(format, title, ext);
char *tmpBuf = new char[nBufLen + 1];
_snprintf_s(tmpBuf, nBufLen + 1, nBufLen, format, title, ext);
//tmpBufで何かする
delete[] tmpBuf;
tmpBuf = NULL;


(A)については議論の余地はないと思います。
十分なサイズを確保するように設計できるなら、
わざわざ面倒くさいsnprintfを使う必要はありません。
1つ問題があるとすれば auto_sprintf_s でこの書き方はできません。

(B)については課題が山積みです。
auto_snprintf_sの引数が足りない、という以前に、
scprintfに相当する実装がありません。
sakuraの事情で「じゃあ作りましょう」とも簡単には言えんです。
でもたぶん、sprintf関数を安全に使う唯一の方法です。


> あとsprintf_sはVC2005では存在していなくて「_sprintf_s」ですよね。

マイクロソフトは独自仕様のCRT関数の先頭に_を付けます。
相手が人なときは省略して説明することが多いです。
相手がマシンなら_sprintf_sって書きますよ。エラーになりますしw

snprintf関数ってものも、昔は存在していませんでしたよ。
だからこそ _scprintf 関数なんてものが作られたわけで・・・。
(_scprintfはsnprintfのbuffに0を指定した場合の動作を提供)


いちおう、次のいずれかの対応を期待して書いているわけです。
1、YES: 内容は理解できたので修正を検討します。
2、NO: sakuraのポリシーに反するので修正は行いません。

2016/11/30 (水) 16:31:39 もか 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
[2388] Re5:セキュアなsnprintf関数。
それ以前の問題として、サクラの定義をよく見てみると、数か所間違いがあるようにみえます。
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;
 }

2016/12/3 (土) 00:36:13 ばぼ 返信 削除
Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; rv:11.0) like Gecko
[2390] Re6:セキュアなsnprintf関数。
▼ もかさん
> それ以前の問題として、サクラの定義をよく見てみると、数か所間違いがあるようにみえます。
> 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版関数を呼び出すだけの代物なので、
呼び出し口で1か所対応すれば目的を果たせるはずです。
邪道を利用してる箇所は非常に少なかったはずなので、
呼出し側を修正して関数定義ごと削除するのが適切かと思います。

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

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


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

[▼次のスレッド]
INCM/CMT
Cyclamen v3.81
[ut:0.010][st:0.000]