roll back Group::Add when duplicate flag detection throws

[metsw24-max]

Dangling child pointer when duplicate detection rejects a flag

Detection runs from the new flag's own constructor, so on a collision the ParseError is thrown before that constructor finishes and the flag's storage goes away during the unwind, while Group::Add has already pushed the pointer into children. Catching the error and carrying on with the parser is then a use-after-free on the next Reset, Parse or Help.

ArgumentParser::AddCompletion has the same shape at a second site: it took the pointer before calling Add, so a rejected CompletionFlag left ArgumentParser::completion dangling too. Assigning after Add succeeds looks sufficient there, since Add never reads it.

Reproduction

#include <args.hxx>
#include <vector>

int main()
{
    args::ArgumentParser parser("test");
    args::Flag a(parser, "a", "a flag", {'a', "alpha"});

    try
    {
        // Registered at run time, and happens to collide with a flag already
        // in the parser.
        new args::Flag(parser, "dup", "dup flag", {'a', "dup"});
    }
    catch (const args::Error &)
    {
    }

    parser.ParseArgs(std::vector<std::string>{"--alpha"});
}

Built with -fsanitize=address against master:

==86928==ERROR: AddressSanitizer: heap-use-after-free on address 0x611000000040
READ of size 8 at 0x611000000040 thread T0
    #0 in args::Group::Reset() args.hxx:1939
    #1 in args::Command::Reset() args.hxx:2651
    #2 in args::ArgumentParser::Reset() args.hxx:3632
    #3 in args::ArgumentParser::ParseArgs<...>(...) args.hxx:3647
    #5 in main repro.cxx:19

0x611000000040 is located 0 bytes inside of 216-byte region
freed by thread T0 here:
    #1 in main repro.cxx:13
previously allocated by thread T0 here:
    #1 in main repro.cxx:13

The AddCompletion half is needed on its own. Swapping the duplicate Flag above for new args::CompletionFlag(parser, {"alpha"}) and applying only the Group::Add change still reports a use-after-free, this time on completion->Matched() in Parse. Both variants run clean with the two together.

test/duplicate_flag_rollback.cxx covers the part that is visible without a sanitiser: the group no longer keeps the rejected child, and the parser still parses afterwards. It fails on master.

Detection cannot throw under ARGS_NOEXCEPT, where it flags a usage error and the child is fully constructed, so the rollback sits behind #ifndef ARGS_NOEXCEPT and the -fno-exceptions build is unaffected.

[Taywee]

This looks like a good fix, though with normal use of the library (where any exception in a flag is caught outside the scope of the parser) this wouldn't end up being an issue. This usage here could be useful for specialty cases, though, like wrapping this library from a dynamic language like Lua.

Thanks for the PR.

[metsw24-max]

Agreed, it mostly bites when the parser outlives a failed registration, so the wrapper case is the realistic one. Thanks for merging.