Vistaスタイルのファイルダイアログ使用時に新規ファイルの保存が行えない問題を修正

[beru]

#862 で報告した問題を解消する修正を行いました。

従来から存在する CommonFileDialog を使った実装ではファイル拡張子の補完処理を自前で色々やっていますが、Vistaスタイルのファイルダイアログ（CommonItemDialog）の実装では同様の処理を端折っている為に再現は出来てません。

sakura/sakura_core/dlg/CDlgOpenFile_CommonFileDialog.cpp

Line 402 in 4edee2b

// 拡張子の補完を自前で行う // 2006.11.10 ryoji

[berryzplus]

呟きです。指摘の意図はありません。
const指定を外すなら何文字まで書き込みできるかの情報を渡してバッファオーバランを抑止したいです。
引数を増やすのは嫌なので std::basic_string を参照渡しにしたらいいんじゃないかと思いました。
パス文字列なのでW⇒A⇒Wのように変換を繰り返すと情報が失われるリスクがあると思います。
個人的にはパラメータを std::wstring& 型に変えるのが安全策なのかな、と思います。

[beru]

確かにバッファオーバーランを防ぐために引数を追加した方が良いですね。

ただ根元の CDlgOpenFile::DoModal_GetSaveFileName に何文字まで書き込めるか（もしくはバッファ長？）のパラメータが無いので、付けるとしたらそこから付けたいです。あと今調べてみたら呼び出し元での配列宣言時の長さは _MAX_PATH だったり _MAX_PATH + 1 でした。終端 0 を含むのか含まないのかどっちか謎です。。

std::wstring& に変えるのも手だと思いますがそれをやるとしてもやはり根元のCDlgOpenFile::DoModal_GetSaveFileName からの変更にしたいです。あと std::wstring にするのはメモリの動的確保を強制する事になるので、std::wstring_view の方が良いと思います。

あと実際にやるとしても、もしバッファーオーバーランする不具合が今無いのであれば別PRでの対処にしたいです。

[berryzplus]

確かにバッファオーバーランを防ぐために引数を追加した方が良いですね。

ただ根元の CDlgOpenFile::DoModal_GetSaveFileName に何文字まで書き込めるか（もしくはバッファ長？）のパラメータが無いので、付けるとしたらそこから付けたいです。あと今調べてみたら呼び出し元での配列宣言時の長さは _MAX_PATH だったり _MAX_PATH + 1 でした。終端 0 を含むのか含まないのかどっちか謎です。。

あ、なので「修正して」とは言いづらいっていうですね:smile:

ファイルパス長を表す固定値MAX_PATHには終端NULが含まれます。
char szPath[MAX_PATH];とやったときに十分なバッファサイズが確保できるようにそうなってるっぽいです。
サクラエディタのコード内にある _MAX_PATH + 1 は　単に誤り　だと思います。
Windows関係のエロい人が薄かった時期があったようなので、その頃にコピペとかで拡散したんじゃないかと思います。

ググってみたらこんな記事が見つかって吹きました。　⇒ Windows 10のデフォルトのパス長制限（MAX_PATH）は256文字です
_MAX_DIR=256という定義があるので、それと混同してるんじゃないかと思います。

std::wstring& に変えるのも手だと思いますがそれをやるとしてもやはり根元のCDlgOpenFile::DoModal_GetSaveFileName からの変更にしたいです。あと std::wstring にするのはメモリの動的確保を強制する事になるので、std::wstring_view の方が良いと思います。

あと実際にやるとしても、もしバッファーオーバーランする不具合が今無いのであれば別PRでの対処にしたいです。

あ、なので「修正して」とは言いづらいっていうですね... 😢

このPRに関していうと、PWSTR pszFilePathを_tcscpyでTCHAR* pszPathにコピーしている点が気になっています。
TCHAR*のままで行くなら to_tchar 関数を噛ませる必要があるように思います。（実際そんな必要はない、ってのはto_tcharの関数定義を見れば分かる通り。

サクラエディタで書き込み可能文字列の参照型を扱うとしたら、CDataProfile.h に定義されている CStringBufferW 型を流用できないかなぁ、と思っています。
std::string_view型を使うには /std:c++17 しないといかんはずなので、現状では使えないです。
というか、std::string_viewは読み取り専用コピーなので、const TCHAR* と変わらん（＝書き込みできない）と思います。

GWだし、みんなキャンプとか行ってるのかなぁ...

[beru]

ファイルパス長を表す固定値MAX_PATHには終端NULが含まれます。
char szPath[MAX_PATH];とやったときに十分なバッファサイズが確保できるようにそうなってるっぽいです。
サクラエディタのコード内にある _MAX_PATH + 1 は　単に誤り　だと思います。
Windows関係のエロい人が薄かった時期があったようなので、その頃にコピペとかで拡散したんじゃないかと思います。

お、MAX_PATH は終端 0 を含むんですね、ちゃんと理解してなかったです。調べたところ確かにMSのサイトにもそのように書かれてました。

https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file#maximum-path-length-limitation より引用

In the Windows API (with some exceptions discussed in the following paragraphs), the maximum length for a path is MAX_PATH, which is defined as 260 characters. A local path is structured in the following order: drive letter, colon, backslash, name components separated by backslashes, and a terminating null character.

ググってみたらこんな記事が見つかって吹きました。　⇒ Windows 10のデフォルトのパス長制限（MAX_PATH）は256文字です
_MAX_DIR=256という定義があるので、それと混同してるんじゃないかと思います。

あ、本当だ。間違ってますね。256 というのがきりが良くて覚えやすい数字なので脳みそが勝手に情報を圧縮して切り詰めちゃうんでしょうね。自分も MAX_PATH が 260 というのは覚えていなかったです。

std::wstring& に変えるのも手だと思いますがそれをやるとしてもやはり根元のCDlgOpenFile::DoModal_GetSaveFileName からの変更にしたいです。あと std::wstring にするのはメモリの動的確保を強制する事になるので、std::wstring_view の方が良いと思います。
あと実際にやるとしても、もしバッファーオーバーランする不具合が今無いのであれば別PRでの対処にしたいです。

あ、なので「修正して」とは言いづらいっていうですね... 😢

短期的にはまずきちんとファイルダイアログが機能していない問題を直したいです。

このPRで呼び出し元から含めて体裁を整えるというかより良いコードにする修正した方が良いとレビューワーが判断したなら、実施するのはやぶさかでは無いですがレビュー範囲が増えるという事は認識ｼﾃｸﾀﾞｻｰｲ（ペリーの口調

このPRに関していうと、PWSTR pszFilePathを_tcscpyでTCHAR* pszPathにコピーしている点が気になっています。
TCHAR*のままで行くなら to_tchar 関数を噛ませる必要があるように思います。（実際そんな必要はない、ってのはto_tcharの関数定義を見れば分かる通り。

実質問題ないかなと思いますが論理的には問題あるので直しておきます。

サクラエディタで書き込み可能文字列の参照型を扱うとしたら、CDataProfile.h に定義されている CStringBufferW 型を流用できないかなぁ、と思っています。
std::string_view型を使うには /std:c++17 しないといかんはずなので、現状では使えないです。
というか、std::string_viewは読み取り専用コピーなので、const TCHAR* と変わらん（＝書き込みできない）と思います。

あ、確かに。string_view であって string_ref では無いんですね。うーん、制限が掛かってるんですね。CStringBufferW を使えないかは確認してみます。

GWだし、みんなキャンプとか行ってるのかなぁ...

海外旅行に行ってる人も多そうですね。

[berryzplus]

呟き。TCHARじゃダメじゃん:cry:

[beru]

ここ、よくよく考えてみると GetDisplayName で取得するパス長がとても長い可能性があるので、wcscpy だと危ないですね。ただ出力先の引数 pszPath のバッファ長が引数に無くて分からないので何文字までなら書き込めるのかもわからないのでエラー処理も入れられないです。

これに関してはこのPRでは扱わないで新規Issueを作る事にします。

[beru]

#881 を作成しました。厳密にはリファクタではなくて CDlgOpenFile_CommonItemDialog の実装の不具合に近いですが、変更範囲が大きくて変更に時間が掛かるので別PRで対応したいです。

[berryzplus]

判断保留です。
インターフェースを変えたほうがいいような気がするんですけど影響範囲が見切れてないので「修正してください」とも言えない感じです。

[berryzplus]

どうしましょう。

伝統的（？）には wcsncpy_s に size=MAX_PATH、count=_TRUNCATE を指定することで 不正なパラメータ例外(=キャッチ不可で即死するプログラムエラー) を回避してきたようです。独自関数auto_snprintf_s の実装がそんな感じになってたはずです。
https://docs.microsoft.com/ja-jp/cpp/c-runtime-library/reference/strncpy-s-strncpy-s-l-wcsncpy-s-wcsncpy-s-l-mbsncpy-s-mbsncpy-s-l?view=vs-2019

関数仕様上は文字列が切り詰められたかどうかを戻り値で判定することができるので、あとあとエラー処理を組み込むことは可能と思われます。(文字列系クラスに operator = を実装して、オーバーフローしたら例外投げる、例外はどこかでまとめてキャッチして良きに計らうという別案をどこかのタイミングで投下するつもりでした。)

おそらく、現時点で_sにしない理由は 不正なパラメータ例外による即死 を懸念してのものだと思います。まぁ、ぼくも一晩寝て考えてみることにします。

[beru]

伝統的（？）には wcsncpy_s に size=MAX_PATH、count=_TRUNCATE を指定することで 不正なパラメータ例外(=キャッチ不可で即死するプログラムエラー) を回避してきたようです。独自関数auto_snprintf_s の実装がそんな感じになってたはずです。
https://docs.microsoft.com/ja-jp/cpp/c-runtime-library/reference/strncpy-s-strncpy-s-l-wcsncpy-s-wcsncpy-s-l-mbsncpy-s-mbsncpy-s-l?view=vs-2019

関数仕様上は文字列が切り詰められたかどうかを戻り値で判定することができるので、あとあとエラー処理を組み込むことは可能と思われます。(文字列系クラスに operator = を実装して、オーバーフローしたら例外投げる、例外はどこかでまとめてキャッチして良きに計らうという別案をどこかのタイミングで投下するつもりでした。)

呼び出し元まで確認すれば引数で渡している出力先のバッファ長が MAX_PATH っていうのは確認が出来ますが、見る範囲を狭めると出力先のバッファ長が明示的に指定されていないので不明なんですよね。なのでそういう状況で safe string function に MAX_PATH って指定するのは関数外の実装を前提にしてしまう事になってあまり宜しくないかなと思います。

おそらく、現時点で_sにしない理由は 不正なパラメータ例外による即死 を懸念してのものだと思います。まぁ、ぼくも一晩寝て考えてみることにします。

即死を懸念していたわけではなかったです。仮に出力先のバッファ長が MAX_PATH っていう事を前提にするとして、コピー元の文字列長は wcslen で確認出来るので長すぎた場合に切り詰めたりしないでエラーで操作失敗にした方が良いのかもしれないと思ってます。ファイルダイアログを閉じる前にユーザーに警告表示するべきかもしれませんが…。OPENFILENAMEW::nMaxFile 的なのが Common Item Dialog にも有るか分かりませんが調べてあるならばそれを使うとか…。

まぁ色々考えると対処が込み入りそうなので、このPRでまずは致命的な不具合を直して、LFN対応がきちんと出来ていないのは別PRで対処したいです。

[beru]

safe string functions に関しては確かMSが提唱して使用を推奨していますが、他のプラットフォームでもみんな使う方向に動いているかというとそんな事も無いような気がします。gcc とか clang とかで同じような警告ってデフォで出してこないですよね？もし他のメジャーなコンパイラでも同様の警告をデフォで出してくるなら従った方が良いと思います。

引数指定をむやみやたらに増やす変更を全体に対して適用するのはちょっと良くない気がします。とはいえポリシーで安全第一という事で使用を強制された場合は従うしか無いですが…。

文字列やパスの操作はスタック上の配列を使うより、動的なメモリ確保の頻度が増えてしまうとしてもクラスで抽象化して扱うようにした方が良いかなと思ってます。LFN対応もその方がやりやすいと思いますし。

たくさん回るループの中の処理は出来るだけ切り詰めた記述にしたいですが、そうでないなら分かりやすさ優先で書きたいです。

長々と書きましたが、自分が safe string functions の全体的な使用を受け入れたくないのは書くのが面倒だからだと思います。横着な姿勢がバグに繋がるのは否定出来ないですが楽して記述が出来ると良いなと思います。

[berryzplus]

外出先から帰ってまいりました。平成最後の夜です。・・・LGTMです。

現状の問題は保存ダイアログを開いた結果の「保存先パス」を内部変数に回収できていないことですが、このPRによってまず「ユーザー入力を変数に格納する」が実現できる認識です。

第二段階として「ユーザーが選択したパスによってはバッファオーバーランが起きるんじゃないか？」という懸念があるわけですが、これについては別に考えて「余裕があればやる」というスタンスでいいように思います。

[beru]

レビューありがとうございます。Mergeします。

バッファ長を引数で渡すようにする改善（もしくは文字列バッファクラス CStringBufferW を使う）と、ユーザーが選択したパスによってはバッファオーバーランが起きるかもしれない問題への対処は別PRで行います。