無為空間
むいむい(´ω｀*)
Entries
ぱっと見どこかに問題ありそうなのだが、その実なんとか動きそうで、でもやっぱり欠陥を含んでいるという、なかなか挑発的なコードに出会った。
開発環境はVC8。構造体アライメントは8バイト。バイトオーダーはリトルエンディアン。
unsigned char _buffer[27]; ///< データバッファ
まず27バイトのデータバッファがある。
/// データバッファ範囲
struct BufferRange
{
size_t offset; ///< データバッファオフセット
size_t size; ///< データサイズ
};
/// データバッファ格納データ範囲一覧
const BufferRange BUFFER_RANGES[] = {
{ 0, 10 }, // データHoge用バッファ範囲（0?9）
{ 10, 16 }, // データFuga用バッファ範囲（10?25）
{ 26, 1 }, // データPiyo用バッファ範囲（26）
};
/// データバッファ格納データインデックス一覧
enum BufferIndex {
BUFFER_INDEX_HOGE = 0, ///< データHogeインデックス
BUFFER_INDEX_FUGA, ///< データFugaインデックス
BUFFER_INDEX_PIYO, ///< データPiyoインデックス
};
void Store(BufferIndex index, void* data)
{
const BufferRange& range = BUFFER_RANGES[index];
::memcpy(_buffer + range.offset, data, range.size);
}
データバッファには
- データHoge（10バイト）
- データFuga（16バイト）
- データPiyo（1バイト）
の3つのデータを格納することになっている。
// データHoge構造体
struct Hoge
{
long val1; // 4バイト
long val2; // 4バイト
short val3; // 2バイト
};
void StoreHoge(const Hoge& hoge)
{
Store(BUFFER_INDEX_HOGE, &hoge);
}
StoreHoge関数は構造体Hogeの内容をデータHoge用バッファにコピーする。
構造体Hogeのサイズは4バイト・4バイト・2バイトに境界調整（JIS X 3014:2003 §3.9/5）用2バイトを含めて12バイト。
それに対しデータHoge用バッファのサイズは10バイトで、コピーするバイト数も10バイト。
一見、バッファにコピーするバイト数が足りていないように見える。
が、コピーされない2バイトは境界調整用の空き領域のため、最低限必要な情報量はコピーされる。
void LoadHoge(Hoge& hoge)
{
const BufferRange& range = BUFFER_RANGES[BUFFER_INDEX_HOGE];
::memcpy(&hoge, _buffer + range.offset, sizeof(hoge));
}
LoadHoge関数はデータHoge用バッファから構造体Hogeにデータをコピーする。
この関数はStoreとは逆に2バイト余計に（データHoge用バッファから10バイト、データFuga用バッファから2バイト）コピーしているが、余計な2バイトは構造体末尾の境界調整用の空き領域にコピーされるため問題は起こらない。
// データFuga構造体
struct Fuga
{
long val1; // 4バイト
long val2; // 4バイト
long val3; // 4バイト
};
void StoreFuga(const Fuga& fuga)
{
Store(BUFFER_INDEX_FUGA, &fuga);
}
構造体Fugaのサイズ（12バイト）がデータFuga用バッファのサイズ（16バイト）より小さいという、Hogeの逆パターン。
一見、書き込み先の方が大きいためバッファオーバーフローの危険はないように見える。
が、逆に書き込み元が小さいためバッファオーバーリードが起こる。
今回書き起こした例は4バイトしかオーバーしないため問題が顕在化することはないかもしれない。
実際のコードでは30バイトほどオーバーしていたため、まれに読み込み可能領域外まで読みに行ってAccess Violationを起こすことがあった。
void LoadFuga(Fuga& fuga)
{
const BufferRange& range = BUFFER_RANGES[BUFFER_INDEX_FUGA];
::memcpy(&fuga, _buffer + range.offset, sizeof(fuga));
}
Loadは問題ない。
// データPiyo列挙体（最大情報量1バイトの符号無し整数）
enum Piyo {
PIYO1,
PIYO2,
PIYO3,
};
void StorePiyo(Piyo piyo)
{
Store(BUFFER_INDEX_PIYO, &piyo);
}
列挙体Piyoのサイズは処理系依存（JIS X 3014:2003 §7.2/5）だが、今回の環境ではintと同じ4バイト。
それに対してデータPiyo用バッファのサイズは1バイトで、コピーするバイト数も1バイト。
一見、バッファにコピーするバイト数が足りていないように見える。
が、バイトオーダーがリトルエンディアンなので、「コピーする先頭1バイト=下位1バイト=データPiyoの全情報」となり、先頭1バイトだけで必要十分な量のコピーが行われることになる。
void LoadPiyo(Piyo& piyo)
{
const BufferRange& range = BUFFER_RANGES[BUFFER_INDEX_PIYO];
piyo = static_cast<Piyo>(_buffer[range.offset]);
}
データバッファの1バイトを列挙型Piyoにstatic_castする。
列挙型の列挙値の範囲にある場合、値は変換によっても変化しないが、そうでない場合の結果の列挙値は規定されていない（JIS X 3014:2003 §7.2/9）。
データバッファの型が符号付き型でかつ列挙型Piyoに128以上の値が与えられた列挙子があったら（符号付き型では負数になるため）問題になるところだが、幸いデータバッファの型は符号無し型なので、ここは特に問題にはならない。
ほかが酷いためつい警戒してしまうが、これはそんな目くじらを立てるようなコードでもないか。
……とまあ、こんな感じのコードが延々と続く。
環境依存であることを理解した上で敢えてこう書いているのか、はたまた「動くようにがんばったらこうなった」のか。
いずれにしろレビューではじくべきコードである。
// コンパイルオプション /DUNICODE 付き HANDLE event = ::CreateEvent(NULL, FALSE, FALSE, (LPCTSTR)"HOGE");
最近というか、今年に入ってから書かれたコードでこの「マルチバイト文字列からUNICODE文字列ポインタへの強制キャスト」に2回も出会っている。
しかもレビューをくぐり抜け、結合段階まで行っている。
Cスタイルキャストの愛用者にはこのキャストが問題ないように見えるのだろうか？
いや、逆か。問題があることを理解しないからCスタイルキャストを使い続けているのか。
Hoge* _hoge;
public:
Fuga()
{
_hoge = new Hoge;
}
~Fuga()
{
delete (Hoge*)_hoge; // 意味のないキャスト
}
delete時にキャストする必要があるのは
「仮想デストラクタを持たない基底クラスのポインタから適切なポインタにダウンキャストしなければならない」
か
「void*ポインタ（あるいはそれに相当する整数値）から適切なポインタにキャストしなければならない」
という危険な設計の場合のみ。
それ以外の無意味なキャストは可読性と保守性を下げ、無駄にバグを埋め込める場所を増やす役割しか持たないことを理解していれば、こんなキャストを書く気にはならないはずなのだ。
// クラス定義
class Foo
{
public:
static Foo* GetInstance();
static void Init();
static void Function();
// 以下、静的メンバ関数が続く
private:
Foo() {}
~Foo() {}
Foo(const Foo&);
Foo& operator=(const Foo&);
static Member1 member1;
static Member2 member2;
// 以下、静的データメンバが続く
};
// 呼び出し側はこんなコード Foo::GetInstance()->Function();
コンストラクタとデストラクタが非公開で、インスタンスを取得できるのは唯一静的メンバ関数からのみ。「シングルトンパターンを知ったので使ってみました」的なコードだが、そんなことよりまずはクラスとインスタンスの違いを認識しろと。
クラスFooには静的メンバ関数（クラスメソッド）しか存在せず、静的ではないメンバ関数（インスタンスメソッド）が1つも存在しない。だからそもそもシングルトンにする（インスタンスの数を1つに制限する）とか以前に、インスタンスを作る意味がない。
// わざわざ Foo::GetInstance() しなくとも…… Foo::Function();
「メンバをことごとくstatic」にした上さらに「シングルトンにする」など、そうそうやることではない。
やるとしたら……「クラスとインスタンスの違い」をさほど重要視しておらず、「コード変更量を最小に抑える」ことをポリシーにしている人だろうか。
そういう人なら、
- 静的メンバだけのクラスFooを作る。
- 最初に1度だけ Init() が呼ばれるよう、シングルトンぽくする。
- 静的メンバのstaticを外さなくても、ちゃんと GetInstance() さえ呼んでくれれば動作に支障は生じないので、無駄にコード変更量を増やさないためにstaticは外さない。
なんてことをやってもおかしくはない。勘弁してほしいが。
class String : std::basic_string<char>
{
public:
operator const char* () const;
};
String CreateVerboseName(const char*);
#if 0
const char HOGE[] = "hoge"
#else
#define HOGE CreateVerboseName("hoge")
#endif
void foo()
{
char hoge[sizeof(HOGE)];
sprintf(hoge, HOGE);
絶賛稼働中のシステムに含まれていたコード。
「関数foo()の追加」と「HOGEの定義変更」は、ほぼ同時期行われている。
おそらく、複数人による変更が嫌な具合に噛み合い、このような形になってしまったのだろう。
明らかに誤ったコードではあるが、 CreateVerboseName("hoge") は sizeof(String) （VC8で32）文字未満の文字列しか返さないので、動作に支障はでていない。
struct Object
{
int point_count;
int* xpoints;
int* ypoints;
// 他省略
};
int* get_point(Objects* obj, int index, int* point)
{
if(index > obj->point_count - 1)
{
point[0] = obj->xpoints[obj->point_count];
point[1] = obj->ypoints[obj->point_count];
return point;
}
point[0] = obj->xpoints[index];
point[1] = obj->ypoints[index];
return point;
}
「indexが総座標数を超えている場合は最後の座標を返す」つもりなのだろうが、これでは「最後の座標の次」を返そうとして、確保したメモリ領域の外を参照してしまう。
たとえ最後の座標を返すようになっていたとしても、総座標数が0の時にはどうしようもない。
コーディングもミスっているが設計もミスっている。
get_center( get_point(obj, index1, point), get_point(obj, index2, point), center_point );
C言語を知っている人なら、get_point()の定義とこの行を示されれば、何か問題がありそうと感づくことだろう。
初歩的ゆえにありがちなミス。
void clear_objects(std::vector<Object> objs)
{
objs.clear();
}
C++を知っている人なら(ry
ここだけ見ると単に参照記号を付け忘れただけのようだが、コード中にvectorの参照渡し（ポインタ渡し含む）が存在せず、数千の要素を含むvectorを頻繁にコピーしているところを見るに、何か宗教上の理由によりvectorを参照渡しできない人が書いたのかもしれない。
こんな感じのコードが延々と続く。
これが「新人に実装させてみてレビューを控えているコード」とかいったものであればどうということはない、これを踏まえて次頑張れってなものだが、「既に正式に使われているプログラムのコード」であるからなんともはや。
「酷いコード」云々以前に、「酷い開発体制」である。
