#106 Helper function IsRectEmpty() is passing a temporary SgRect object to iterator

[Patrick Näf]

In GoUctUtil.cpp there is the helper function IsRectEmpty() with the code currently looking like this:

bool IsRectEmpty(const GoBoard& bd, int left, int right, int top, int bottom)
{
    for (SgRectIterator it(SgRect(left, right, top, bottom)); it; ++it)
        if (! bd.IsEmpty(*it))
            return false;
    return true;
}

The temporary SgRect object passed to the SgRectIterator constructor is a problem. If the SgRect object goes out of scope before the full for-loop has run through, the iterator will iterate over an undefined set of intersections - the SgPoint objects produced by the iterator may even reference intersections that are outside the board. This may result in IsRectEmpty() returning the wrong result.

This is not a theoretical issue, it can be observed in real life when Fuego is compiled on macOS with the latest Xcode 10.1, i.e. using the packaged Clang compiler (clang-1000.11.45.5), and the target is either the iOS simulator (Intel platform) or a real iOS device (ARM platform) . A notable compiler option that might affect the discussion is -std=gnu++98. Note that I was unable to reproduce the problem when using the same compiler but compiling for the macOS platform itself!

Here are the runtime repro steps:

• Start a game on a board >= 13x13 with handicap >= 2. This means that there are at least two stones on the board, on two of the 4/4 intersections.
• Execute the GTP command "genmove" as the first or second move of the game.
• The GTP command causes the function GoUctUtil::GenForcedOpeningMove() to be called, this in turn calls the problematic IsRectEmpty() helper function one or more times in order to determine whether it is allowed to place a stone on one of the 4/4 intersections.
• IsRectEmpty() erroneously returns true for a rectangle which, in reality, contains a handicap stone.
• As a result GoUctUtil::GenForcedOpeningMove() then attempts to place a stone on the 4/4 intersection for which it called IsRectEmpty(), but since that is already occupied by a handicap stone the "genmove" GTP command fails. The GTP error message looks like this (example is from a 15x15 board): "GoUctPlayer generated illegal move: 3 B D12 (occupied)".
• The problem can also be reproduced in a non-handicap game, when the first move plays a stone on a 4/4 intersection, and the second move is generated by "genmove".

So is this really a bug in Fuego? Isn't this a Clang bug? After all the code worked flawlessly for many years. In my opinion these questions lead the discussion in the wrong direction. I think it's safe to say that using a temporary object in the way that the current code does is fishy at best. To make an analogy: Would you create, for instance, a temporary std::vector in the constructor call of the iterator that is supposed to iterate over the very same std::vector? I think not.

So instead of discussing which compiler got the C++ standard's scoping rules right, I propose a simple code change that gets rid of the temporary object and makes the code clear and unambigous:

bool IsRectEmpty(const GoBoard& bd, int left, int right, int top, int bottom)
{
    SgRect rect(left, right, top, bottom);
    for (SgRectIterator it(rect); it; ++it)
        if (! bd.IsEmpty(*it))
            return false;
    return true;
}

The attached patch produces the code above. It was created with svn diff gouct/GoUctUtil.cpp.

[Martin Müller]

I agree and have fixed it