Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Apr 30, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d5392a
Vistaスタイルのファイルダイアログ使用時に新規ファイルの保存が行えない問題を修正
beru Apr 27, 2019
8e91d8f
ワイド文字を扱う個所において、_tcscpy ではなくて wcscpy を使うように修正
beru Apr 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions sakura_core/dlg/CDlgOpenFile_CommonItemDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ struct CDlgOpenFile_CommonItemDialog final
std::vector<std::tstring>* pFileNames,
LPCWSTR fileName,
const std::vector<COMDLG_FILTERSPEC>& specs );
bool DoModalSaveDlgImpl0( const TCHAR* pszPath );
bool DoModalSaveDlgImpl0( TCHAR* pszPath );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ただ根元の 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だし、みんなキャンプとか行ってるのかなぁ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ファイルパス長を表す固定値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だし、みんなキャンプとか行ってるのかなぁ...

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

HRESULT DoModalSaveDlgImpl1( IFileSaveDialog* pFileSaveDialog,
const TCHAR* pszPath );
TCHAR* pszPath );

HINSTANCE m_hInstance; /* アプリケーションインスタンスのハンドル */
HWND m_hwndParent; /* オーナーウィンドウのハンドル */
Expand Down Expand Up @@ -731,7 +731,7 @@ bool CDlgOpenFile_CommonItemDialog::DoModalOpenDlg(

HRESULT CDlgOpenFile_CommonItemDialog::DoModalSaveDlgImpl1(
IFileSaveDialog* pFileSaveDialog,
const TCHAR* pszPath)
TCHAR* pszPath)
{
//カレントディレクトリを保存。関数から抜けるときに自動でカレントディレクトリは復元される。
CCurrentDirectoryBackupPoint cCurDirBackup;
Expand All @@ -753,6 +753,7 @@ HRESULT CDlgOpenFile_CommonItemDialog::DoModalSaveDlgImpl1(
specs[2].pszName = strs[2].c_str();
specs[2].pszSpec = _T("*.*");
#define RETURN_IF_FAILED if (FAILED(hr)) { /* __debugbreak(); */ return hr; }
hr = pFileSaveDialog->SetDefaultExtension(_T("txt")); RETURN_IF_FAILED
hr = pFileSaveDialog->SetFileTypes(specs.size(), &specs[0]); RETURN_IF_FAILED
ComPtr<IShellItem> psiFolder;
SHCreateItemFromParsingName(m_szInitialDir, NULL, IID_PPV_ARGS(&psiFolder));
Expand All @@ -765,12 +766,22 @@ HRESULT CDlgOpenFile_CommonItemDialog::DoModalSaveDlgImpl1(
hr = Customize(); RETURN_IF_FAILED
}
hr = pFileSaveDialog->Show(m_hwndParent); RETURN_IF_FAILED

if (SUCCEEDED(hr)) {
ComPtr<IShellItem> pShellItem;
hr = pFileSaveDialog->GetResult(&pShellItem); RETURN_IF_FAILED
PWSTR pszFilePath;
hr = pShellItem->GetDisplayName(SIGDN_FILESYSPATH, &pszFilePath); RETURN_IF_FAILED
_tcscpy(pszPath, pszFilePath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@beru beru Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CoTaskMemFree(pszFilePath);
}

#undef RETURN_IF_FAILED

return S_OK;
}

bool CDlgOpenFile_CommonItemDialog::DoModalSaveDlgImpl0( const TCHAR* pszPath )
bool CDlgOpenFile_CommonItemDialog::DoModalSaveDlgImpl0( TCHAR* pszPath )
{
using namespace Microsoft::WRL;
ComPtr<IFileSaveDialog> pFileDialog;
Expand Down