ぼやきごと/2016-03-04/GetLastError 関数に頼るのは危険 のバックアップ(No.1)

[前へ] [次へ] [目次]

GetLastError 関数に頼るのは危険 †

下記のような関数があるとします。

すべて開くすべて閉じる
  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18

 
 
 
-
|
!
-
|
|
|
-
-
!
|
!
|
|
!

#include <string>
#include <Windows.h>
 
// IDに対応するミューテクスハンドルを返す。
// 既に作成済みの場合は nullptr を返す。
::HANDLE createUniqueMutex(int id)
{
    ::HANDLE mutex = ::CreateMutexW(nullptr, FALSE, std::to_wstring(id).data());
 
    if (mutex != nullptr && ::GetLastError() == ERROR_ALREADY_EXISTS)
    {
        // 既に作成済みなので nullptr を返す
        ::CloseHandle(mutex);
        mutex = nullptr;
    }
 
    return mutex;
}

処理内容は下記の通りです。

  1. 引数に渡した数値を std::to_wstring 関数で文字列に変換する。
  2. 1. の文字列を名前として CreateMutexW 関数を呼び出し、ミューテクスを作成。
  3. GetLastError 関数の戻り値が ERROR_ALREADY_EXISTS ならば既に作成済みのミューテクスなので解放して nullptr を返す。

特に問題はなさそうに見えますが、ある特定の状況だとこの関数は意図通りに動きません。

その状況とは、グローバルの delete 演算子がオーバロードされ、その中で GetLastError 関数の内部値が上書きされた場合です。

すべて開くすべて閉じる
  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15

 
 
 
 
-
|
!
 
 
-
-
!
|
|
!

#include <cstdlib>
#include <Windows.h>
 
void* operator new(std::size_t size)
{
    return std::malloc(size);
}
 
void operator delete(void* ptr)
{
    // GetLastError の内部値を上書きするような処理
    ::SetLastError(0);
 
    std::free(ptr);
}

上記の例では直接 SetLastError 関数を呼んでいますが、実際には何らかの Win32 API が呼ばれているものと思ってください。

先のコードの、 CreateMutexW 関数を呼んでいる文は次の通りです。

  8

 

    ::HANDLE mutex = ::CreateMutexW(nullptr, FALSE, std::to_wstring(id).data());

std::to_wstring(id) で作成した std::wstring は特に変数等に束縛されていないため、 CreateMutexW 関数から処理が戻った時点でデストラクタが呼び出されます。
std::wstring のデストラクタは、内部に保持しているバッファを delete 演算子によって解放します。
よって、 GetLastError 関数を呼び出すよりも前に operator delete が呼び出され、内部値が上書きされてしまいます。

GetLastError 関数を利用するコードと delete 演算子のオーバロードをどちらも自身で書いたならば気付けるかもしれませんが、下記のような場合は非常に気付きにくいです。

  • 外部ライブラリに上述のような GetLastError 関数を利用するコードが入っている場合。
  • 外部ライブラリやフレームワークによってグローバルの delete 演算子がオーバロードされている場合。

グローバルの new, delete 演算子のオーバロードはリンクしているライブラリにまで波及するのがとても厄介です。

両者の組み合わせが一番最悪で、どちらかのソースコードを修正可能でなければどうしようもなくなってしまいます。
私は、自身で作ったライブラリと Unreal Engine 4 （ゲームエンジン）の組み合わせでこの問題にハマり、原因判明までに丸1日を要しました…。

個人的には「グローバルの new, delete 演算子をオーバロードするのはやめろ」と言いたいのですが、外製のフレームワークにやられていたらどうしようもないですし、 GetLastError 関数の仕組みが危険なのもまた事実なので、注意してコーディングするしかないですかね。

なお、今回のコード例であれば、 std::to_wstring(id) の結果を一旦変数に受け取ることで問題を回避できます。
より安全にするなら、最初に OpenMutexW 関数によって既存のミューテクスが存在するか確認するようにすれば、 GetLastError 関数を使わずとも同等の処理が可能です。

Category: [C++][Visual Studio][プログラミング] - 2016-03-04 00:29:24
[前へ] [次へ] [目次]