-
Notifications
You must be signed in to change notification settings - Fork 167
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.
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
RFC5321解釈の誤りを訂正する #493
RFC5321解釈の誤りを訂正する #493
Conversation
sakura_core/util/string_ex.cpp
Outdated
/* | ||
* RFC5321 let-dig の要件を満たすかどうか判定する | ||
* | ||
* 高速化目的で可読性を捨て、あえて冗長な switch 分岐にしている。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可読性の高いバージョンも用意して結果が一致することを確認する単体テストを用意する方がいいです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static関数なので単体テストに入れるのは難しいです。
関数自体をコピペで入れないといけないので。
いちおうテストは書いて、ローカルで実行してみました。
TEST(testIsMailAddress, CheckLetDig)
{
constexpr wchar_t letDigChars[] = L"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
for (wchar_t ch = 0; ch < 0xFFFF; ch++) {
if (ch == 0) continue; //可読性高いverはNUL文字に対応しないので除外する
bool resultOld = (::wcschr(letDigChars, ch) != nullptr);
bool resultNew = IsLetDig(ch);
ASSERT_EQ(resultOld, resultNew);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static関数なので単体テストに入れるのは難しいです。
それなら、static を使わないようにすればいいと思います。
内部利用用の namespace を定義してその中に入れたらいいと思います。
明示的に内部用の namespace を using するか、namespace 指定しない限りアクセスできないです。
例えば googletest は internal
という namespace を使っています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
他関数からの利用を考えなくても良いようにstaticにしてます。
namespaceを付けて公開ヘッダーに移動すると、他関数に流用される危険が出ます。
他関数から利用されてもよいように再設計する必要が出てきます。
現状、コントリビューターに無理な負担を強いない、が基本だと思っています。
基本原則を曲げてまでこの関数のテストを書きたい理由が分からないです。
IsLedDig
は今回追加した中ではかなり単純な部類なので意図を図りかねています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static 関数をテストするために .cpp ファイルをインクルードすることがあるそうです。テストファイルから #include "util/string_ex.cpp"
すれば実質的にはコピペですがコピペではありません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Includeはテスト対象コードを取り込むために使ってました。
気にしてるのは、単体テストを作るコストがテストにより防止できる問題の量に見合うかってところです。
Staticをやめなくていいならテスト書いてもいいです。ただテストを書く意味があるかどうかが分からないです。
7427b54
to
ac814d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pull #421 とは違い高速化を標榜しないこの PR で IsMailAddress が計算量 O(1) を指向しているのは、予想外ですが嬉しいです。
1点確認です。a@v.v.v.v.(長さ制限を丁度超えない程度の長さを省略).v.v.v.v.v.v
という、メールアドレスとしてはちょっとだけ長すぎる文字列があったとして、末尾の .v.v
だけをちょん切ってそれより前をメールアドレスとして判定することをどう捉えるかということです。ちょん切られるのは -vvv
というケースもあります。ドットかハイフンで始まる部分が都合良く切られます。
assert(ppszDotOrNotAlnum != nullptr); | ||
|
||
// サブドメインの後ろに '.' があるかチェックする | ||
if (**ppszDotOrNotAlnum != L'.') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**ppszDotOrNotAlnum
は *pszScanEnd
と等価な場合があると思いますが、pszScanEnd
はアクセスしていい範囲の外では?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sz=null-terminated string なのでアクセスしてはいけない範囲にならない認識です。
BOOL IsMailAddress( const wchar_t* pszBuf, int nBufLen, int* pnAddressLength )
pszScanEnd
の最大値は pszBuf + nBufLen
です。
nBufLenに正しい文字列長が渡ってきているとすればpszBuf[nBufLen - 1] == L'\0'
です。
なので、直接問題にはならないコードだと思っています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pszBuf[nBufLen - 1] == L'\0' です。
pszBuf[nBufLen]
にアクセスするケースを問題視しています。
sz=null-terminated string なのでアクセスしてはいけない範囲にならない認識です。
引数名に sz が付いているからってそれを信じることはできません。コンパイルすればオブジェクトコードからは消える、所詮は名前です。そして NUL で終端されていようといなかろうと、その分の +1 が nBufLen に含まれていようといなかろうと、呼び出し側が nBufLen という引数で伝える内容は、pszBuf[0] から pszBuf[nBufLen-1] までがアクセス可能であることと、C 言語の配列に関する仕様から、pszBuf+(nBufLen-1) < pszBuf+nBufLen (※たぶん nBufLen は正の数に限ると思う) という不等号が成り立つということだけです。
たぶん後ろに NUL 文字用のスペースがあるだろうからとアクセスしてもいい理由はありません。バッファ長を渡している以上、NUL で終端する必然性が呼び出し側にはありません。
さらに言えば、検索に関するコードを読んでいたときですが、文字列バッファ内に NUL 文字を含んでいる場合を考慮する旨のコメントがありました。それがバッファの長さを明示的に引数で渡す理由でもあると思います。sz なんて嘘っぱちです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
対策は要らん!というコメントを書きつつ、対策始めてました。
やっぱり問題ありますよね 😢
さらに言えば、検索に関するコードを読んでいたときですが、文字列バッファ内に NUL 文字を含んでいる場合を考慮する旨のコメントがありました。それがバッファの長さを明示的に引数で渡す理由でもあると思います。sz なんて嘘っぱちです。
サクラエディタではNUL文字が「文字列」を終端しないという事実。
ここの話とは全く関係ありませんが、改善すべき「良くない文化」と考えています。
くわしくは文字コード誤判定の対策を打つときに書くつもりでいます。
ここの話に戻すと NUL文字 は「メールアドレスに使えない文字」です。
たとえサクラエディタがNUL文字を終端扱いしないとしても、メールアドレスはNUL文字で終端されるので、NUL文字の手前までをメールアドレスとして解釈できるか否かの話になります。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
対策はお願いします。おそらくちょん切り問題と関係すると思います。
サクラエディタではNUL文字が「文字列」を終端しないという事実。
もはや過去の事実だろうとは思います。過去に事実であったかもやや疑っています。しかし、文字列ポインタと文字列長がセットになった BSTR 型がヌル文字を途中に含みうるように、明示的に文字列の終端を渡すインタフェイスからはそういう必然性を窺うことができます。
都合の悪い状況がありそうです。 前半部分「メールアドレス(255文字) .com」な場合ちぎれていいと 前半部分「メールアドレス(255文字) com」な場合ちぎれると不自然です。 |
日本語が誤っていました。
と書いた意図は、解釈する側(IsMailAddress)の勝手な都合に合わせて切られる、ということでした。そのことが具合がいいという含みはなく、むしろ御都合主義に対するのと同じく改めるべきだという認識を持っていました。
ドメインは末尾ほど重要なので、勝手に後ろの .com をちぎってはいけないと思います。
-com はその直前の文字列と一体となってドメインを構成する部分なので、勝手にちぎってはいけないと思います。 |
自分は特に言及しておらず RFC を読み込んでもいないのでスルーしましたが、アンダースコアがドメインを構成する文字ではないという事情があるのなら、ちぎれるのは問題ないと思います。 |
これはそういう認識です。 対策としては、関数で検出した終端の後ろに、ドメインの一部になりうる文字が続く場合をNGとしました。これにより、途中でぶった切られたように見えることはなくなると思います。 |
ふと、日本語ドメインを含むメールアドレスは、どうなるのか気になりました。 |
@arigayas さん、どっかでかきましたが、 けんち@けんち.みんな なんてのもかけたかと。 https://japanese.engadget.com/2014/01/16/google/ なので、提案したのは、もう頑張るのはやめて、 |
自分で言っておいてアレですが、 |
mailto: の後ろに限れば誤検出を気にせず対応してもよさそうですね。メールソフトには日本語のままのアドレスを渡せるのでしょうか。punycode って初めて知りました。 ちなみに正規表現キーワードで |
#421 (comment) で「上限より1文字だけ長いメールアドレス(候補)をいったんメールアドレスとして受け入れられるようにしておき、あとで非メールアドレスとして弾いていました」と書きましたが、上限に +1 して後で弾く理由がちょん切り対策でした。 |
日本語メールアドレスについては考慮してないです。
こういうアドレスの場合、ゲートウェイ的には 逆説的にいえば、日本語ドメインに関連付けられた英語表記ドメインがあるかインターネットで問い合わせればよいだけの話なので、実現可能だと思っています。ただその前に、元のきっかけが「高速化」なので「インターネット問い合わせなんてありえん!」と思っとりました。 とりあえず高速化するつもりで変なバグ入れた状況を打破したいのが現状です。
結局、上限より1文字長いメールアドレスを見て判定してるのと同じですね。 |
berryzplus さんのやり方はドメインに関する仕様が |
a@a.aをメールアドレスだと思うこととそれが存在するかを検知するのは別かなと、思った点だけコメント。 |
|
||
// 出力値を初期化する | ||
*ppszEndOfMailBox = nullptr; | ||
*ppszDotOrNotAlnum = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ヌルポインタに不用意に書き込まないように、こういう部分にこそ先駆けて assert を置くものではないでしょうか。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
詳細確認します。スタティック関数で落ちない確信があるからアサートしてないはず。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ヌルポインタに不用意に書き込まないように、こういう部分にこそ先駆けて assert を置くものではないでしょうか。
あとで見たとき関数仕様を忘れて困らないように assert 追加しました。
サブドメインの文字数制限を超えないようにドメイン全体の文字数を増やしていくと assert に引っかかりました。
|
いったん保留にします。 自前実装で関数とテスト書いて無駄なバグ作るより、 |
放置っぽく見えますが裏で検討進めております。 これは大枠でクリッカブルURL検出のための処理なので呼び出し元からの変更を模索してるから時間がかかってるという経緯です。 |
・検出した終端にドメインを構成する文字が続く場合はNGにする ・ドメイン解析中の処理に終端チェックを追加
上位からチェック処理に渡す文字列長を上限より1文字長くする。 上位ではチェック処理後に文字数のみを見て最終的な結果を判断するように変更。
追加チェックしなくても大丈夫そうなので削る
bdb917e
to
4747b82
Compare
指摘点1点 #493 (comment) に対応してコンフリクトを解消しました。 |
CodeFactorがNGを出しているのが気になって switch 分岐を共通のテンプレ関数にできないか検討中。。。 |
CodeFactorにより、switch文を使う関数に対して「コード重複」と「過剰な複雑度」が検出されたことへの対策。 IsLetDig関数をやめCRTのisalnumを使うように変更。 ハングアップしたように見えた元凶はIsAtomCharの方で、@以降を判定するIsLetDlgの処理はそれほど高速でなくともよいと判断した。
よし!CodeFactor黙らせた! ←ちょっと嬉しい |
sakura_core/util/string_ex.cpp
Outdated
_In_ const wchar_t ch | ||
) noexcept | ||
{ | ||
return ch <= 0xFF && ::isalnum( ch ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ch <= 0xFF
の条件は
https://ja.cppreference.com/w/cpp/string/byte/isalnum
で説明されている ch の値が unsigned char で表現できず、 EOF とも等しくない場合、動作は未定義です。
への対応ですか?
コードにコメントで理由の説明がほしい感じです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ch <= 0xFF の条件は
https://ja.cppreference.com/w/cpp/string/byte/isalnumで説明されている ch の値が unsigned char で表現できず、 EOF とも等しくない場合、動作は未定義です。
への対応ですか?コードにコメントで理由の説明がほしい感じです。
ch が ASCII文字
かつ ...
という意図で書いたものなので ch <= 0x7F
の誤りです。
↓
2c52d35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pendingになってたコメントに気付いたので放流します...orz
sakura_core/util/string_ex.cpp
Outdated
_In_ const wchar_t ch | ||
) noexcept | ||
{ | ||
return ch <= 0xFF && ::isalnum( ch ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
某掲示板より転記です。
547名無しさん@お腹いっぱい。2019/01/22(火) 01:05:13.63ID:+KEIPYsY0
メールアドレスのisalnumなんだけどさ
厳密にいえばiswalnumを使うべきなんだよな
結果がたまたま同じになるとはいえ気持ち悪さは拭えない
ちょいまちガードコードが0xFFになってない?
それASCIIなら0x7Fの間違いじゃないかな
8ビット目突っ込むとロケール依存やで
後半部分正しい指摘と思うので修正入れます。
sakura_core/util/string_ex.cpp
Outdated
_In_ const wchar_t ch | ||
) noexcept | ||
{ | ||
return ch <= 0xFF && ::isalnum( ch ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ch <= 0xFF の条件は
https://ja.cppreference.com/w/cpp/string/byte/isalnumで説明されている ch の値が unsigned char で表現できず、 EOF とも等しくない場合、動作は未定義です。
への対応ですか?コードにコメントで理由の説明がほしい感じです。
ch が ASCII文字
かつ ...
という意図で書いたものなので ch <= 0x7F
の誤りです。
↓
2c52d35
これ、判断難しいようなら一旦破棄するのが現実的ですかね。 |
異存ありません。その方がユーザーへの影響度合いが小さいと思います。 |
#421をrevertする方法がわかり次第閉じます。 |
コミットが続いていてレビューのタイミングが掴めないという事情もありそうです。一部ケースでの反応低下と、それよりありふれている(ちょっとのテストで発生する)誤判定を天秤にかけて影響度合いを判断しましたが、@berryzplus さんが「バグはもうない。あとはレビューだけだ」と考えるならレビューを待つ手もあると思います。 根本的な話をすると、エディタで行うアドレス判定の価値は「最小の手間で最大の効果を期待するコストパフォーマンス」と「正規表現キーワードでは実現できない厳密さ」のどちらにもあると思いますが、個人的な話をすれば、誤判定に悩まされたことがないので厳密さを求める立場にはありません。 |
あ、リリースを考えているから待てないんですね。その判断は正しいと思います。 |
CSVやJSONなどのデータ規格と比べてメールアドレスの仕様は雑なので、厳密に準拠することのメリットはほぼないと思われます。もともと「RFC規格に対応させたい」という強い意思でやってたわけでもないので、仕切り直したほうがシンプルな議論ができる気がしています。(クリッカブルURLはどうだったらいいんだろう?という議論が要ると思ってます。 can not revert automatically 的なメッセージが出てたのでrevertは手動でやるしかなさそうです。 |
replaced by #792. |
目的
#421 の改善です。
取込済みPRでRFC5321を誤って解釈していたのを訂正します。
誤解釈によって、正しくないメールアドレスをメールアドレスと判定しています。
GitHub移行後に組み込んだバグなので対処は必須と考えています。
経緯
@berryzplus
@ds14050 さん #421 (comment)
@ds14050 さん #421 (comment)
@berryzplus #421 (comment)
@ds14050 さん #421 (comment)
@berryzplus #421 (comment)
@berryzplus #421 (comment)
@k-takata さん #421 (comment)
@berryzplus #421 (comment)