Entries

スポンサーサイト

上記の広告は1ヶ月以上更新のないブログに表示されています。
新しい記事を書く事で広告が消せます。

最近見かけた酷いコード14

タグ: C++ 酷いコード VS2005

ぱっと見どこかに問題ありそうなのだが、その実なんとか動きそうで、でもやっぱり欠陥を含んでいるという、なかなか挑発的なコードに出会った。
開発環境は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);
}

データバッファには

  1. データHoge(10バイト)
  2. データFuga(16バイト)
  3. データ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以上の値が与えられた列挙子があったら(符号付き型では負数になるため)問題になるところだが、幸いデータバッファの型は符号無し型なので、ここは特に問題にはならない。
ほかが酷いためつい警戒してしまうが、これはそんな目くじらを立てるようなコードでもないか。

……とまあ、こんな感じのコードが延々と続く。
環境依存であることを理解した上で敢えてこう書いているのか、はたまた「動くようにがんばったらこうなった」のか。
いずれにしろレビューではじくべきコードである。

スポンサーサイト

コメント

コメントの投稿

コメントの投稿
管理者にだけ表示を許可する

トラックバック

トラックバック URL
http://idlysphere.blog66.fc2.com/tb.php/255-62213c35
この記事にトラックバックする(FC2ブログユーザー)

Appendix

タグ

Blog内検索

上記広告は1ヶ月以上更新のないブログに表示されています。新しい記事を書くことで広告を消せます。