Why does vector.erase() run into SIGABRT?

Question

My filepath normalization function has a weird issue I'm not fully understanding and am having trouble fixing (I'm also not very seasoned in C++).

/**
 * Converts any path (e.g. /a/d/../b/.//c/) to absolute /a/b/c format.
 * @param path Any valid path beginning with /
 * @return Path in absolute /a/b/c format.
 */
static std::string normalizePath(std::string path)
{
    if (path == "/")
        return "/";

    if (path[0] != '/') // full relative paths not supported due to lack of context
        return "";

    std::vector<std::string> segments = strsplit(path, '/');
    while (segments[0] == "." || segments[0] == "..")
        segments.erase(segments.begin());

    for (int i = 0; i < segments.size(); i++)
    {
        if (segments[i] == "." || segments[i].empty())
            segments.erase(segments.begin() + (i--));
        else if (segments[i] == "..")
            segments.erase(segments.begin() + (--i), segments.begin() + (i+2)); // SIGABRT
    }

    std::string r;
    for (int i = 0; i < segments.size(); i++)
        r += "/" + segments[i];

    return r;
}

It works fine with most inputs, but the input "/a/.." (which is supposed to return "/") makes it crash with SIGABRT at the indicated line.

My understanding is I'm deleting the current and previous element, but apparently that assumption is wrong.

I'm also reluctant to just use realpath() because I'm working with virtual paths and I definitely don't want any calls to any filesystem.

Why does my code crash? How do I make it work as intended?

Answer 1

This line has undefined behavior, because it accesses i twice in the context where the accesses are unsequenced with respect to each other:

segments.erase(segments.begin() + (--i), segments.begin() + (i+2));

Since the order of evaluation is unspecified, and the order of applying side effects is unknown, segments.begin() + (i+2) could evaluate to an iterator past vector's end.

You can fix this by using the value of i without pre-decrement, and applying -- after returning from erase:

else if (segments[i] == "..") {
    segments.erase(std::next(segments.begin(), i-1), std::next(segments.begin(), i+1));
    --i;
}

Note: The above code uses std::next instead of adding numbers to iterators.