CMemory::AppendRawData()でAllocBuffer条件ミスを修正 #1638
Conversation
| @@ -227,7 +227,7 @@ void CMemory::SetRawDataHoldBuffer( const CMemory& cmemData ) | |||
| void CMemory::AppendRawData( const void* pData, size_t nDataLen ) | |||
| { | |||
| // メモリが足りなければ確保する | |||
| if( m_nDataBufSize <= m_nRawLen + nDataLen ){ | |||
| if( m_nDataBufSize <= m_nRawLen + nDataLen + sizeof(wchar_t) ){ | |||
There was a problem hiding this comment.
元々<=だったのでそのままにしてありますが、別に<でもいいですね。
There was a problem hiding this comment.
m_nRawLen + nDataLen + sizeof(wchar_t) はいったんローカル変数に代入して下の判定式でもそのローカル変数を使う記述に変更するのはどうでしょうか？
同じ値の計算を別々に書いていてきちんと一致が取れていなくて出た不具合の対処なので、回避方法としてはローカル変数を使って同一の値を参照するようにするのが良いかなと思いました。
とはいえそういう事をしても可読性が向上せず不具合が続出するのが常かもしれませんが、その時はその時で ＼(^o^)／
There was a problem hiding this comment.
同じ目的の判定が AllocBuffer の中でもされているようですので、いっそ判定をなくしてしまっても良いのかなと思ってしまいました。
sakura/sakura_core/mem/CMemory.cpp
Lines 149 to 151 in 9713af2
There was a problem hiding this comment.
たしかに値をまとめたいところですが、AllocBufferの実行により変更されるので、上と下で中身の数値が異なることがあります。
自分も下のバッファ長チェックは助長に感じます。
AllocBufferの呼び出し条件とAllocBufferのコードが正しいのであれば、メモリーが足りなくてnullptrになる時でなければ成立しない条件のはずです。
今回はセーフネット的にここに引っかかって、書き込まれず露呈したので、完全に無意味とは言いません。
バッファ長チェックはassertで引っ掛けて確認するのはありだと思いますが、毎回リリースでも確認する必要性は感じないです。
There was a problem hiding this comment.
可読性を考慮すると、CMemory::m_nDataBufSizeではなくCMemory::capacity()と比較すべきでしたね。
CMemory::capacity()はstd::string::capacity()と同名意義で分かりづらいのでリネームすべきと思います。
AllocBufferの条件式とここの条件式は意味が違うので、重複じゃないっす。
|
Kudos, SonarCloud Quality Gate passed! |
|
✅ Build sakura 1.0.3694 completed (commit 4ae32f9085 by @usagisita) |
There was a problem hiding this comment.
問題無いと思います。
動作確認して不具合が解消された事を確認しました。
なお動作確認には https://egg.5ch.net/test/read.cgi/software/1587603412/301 のテキストを使っていて他のデータでは動作確認していません。
人間が注意するのは大変だし、CIで色々なデータで動作確認をしていないのでこういう不具合が出ちゃいますね。
|
さらに直すのはありですが、取り急ぎ、一旦この状態でマージしたいと思います。 |
|
レビューありがとうございます。 |
PR の目的
行データが読み込まれないことがある不具合を修正します。
#1618 での変更による不具合です。
5ch情報part19 297-301より
カテゴリ
PR の背景
PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
CMemory::AppendRawDataでAllocBufferを実行しなければならない条件でsizeof(wchar_t)を足すのが抜けているため、
ぎりぎり既存バッファに入らないと、
後続の文字列データの追加の部分の条件式ではねられて、追加されない状態になります。
ファイル読み込み、およびGrepでは
sakura/sakura_core/io/CFileLoad.cpp
Line 391 in 9713af2
ここの呼び出しで発生しています。
PR の影響範囲
ファイル読み込み。CMemoryが使われている箇所ほぼすべて。
ファイル読み込みでは、内部行バッファが拡張されるタイミングで発動する不具合なので、上のほうが正常に読み込まれているように見えても、
ファイルの途中の行でも、後半が切れたりする可能性があります。
特定行が空になる場合は、それ以降の全データが切れて読み込まれます。
この不具合を持ったバージョンを使用するのは強く非推奨です。
テスト内容
テスト1
手順
ASCII5文字+CRLFの合計7Byteが1行のファイルを用意します。
UTF-8(BOMなし)/SJISなどで読み込みます。
修正前は「中身が空の状態」でファイルが読み込まれます。
修正後はファイルの中身が表示されます。
関連 issue, PR
#1618 #1641
参考資料