json.d: union pointer access is unsafe

[WalterBright]

Having a union with overlapping pointers is unsafe to write to. This is a blocker for dlang/dmd#5940 please do not delay review.

[JackStouffer]

You should get rid of the trusted lambda on line https://github.com/dlang/phobos/pull/4608/files#diff-7441f65d4929cc6c45a3094b58973fcdR353

[WalterBright]

That's something different. I think it's best to leave that in.

[JackStouffer]

If you're making the whole function trusted there's no reason to keep in the trusted work arounds.

You should either wrap everything unsafe in a trusted lambda or make the whole function trusted, but there's no reason to do both.

[dnadlinger]

@WalterBright:

struct Oops {
  long oops() { return *(cast(long*)0xdeadbeef); }
  alias oops this;
}

long misplaceTrust(T : long)(T t) @trusted { return t; }

void innocent() @safe { misplaceTrust(Oops.init); }

The types passed to assign do not seem to be constrained (see class constructors).

[WalterBright]

The types passed to assign do not seem to be constrained (see class constructors).

I don't know what you mean.

[WalterBright]

BTW, the purpose of this PR is not to re-engineer json. It's simply to get the blocker removed so I can continue work on fixing the safe bugs in the compiler.

[aG0aep6G]

Can't put @trusted on a template like that. T may be a struct with an unsafe alias this, postblit, or destructor.

[dnadlinger]

@WalterBright:

The types passed to assign do not seem to be constrained (see class constructors).

I don't know what you mean.

You marked a function as @trusted that appears to receive arbitrary types (I didn't double-check, hence the disclaimer, but the constructors seem to take an arbitrary T and forward it to assign).

The snippet I showed demonstrates why this is problematic – assign might execute an arbitrary alias this in a trusted scope.

[andralex]

Yes, the code is in error. @WalterBright please fix, thx.

[PetarKirov]

So we should make the @safe-ty of assign depend on the alias this and then set the union member in a separate @trusted lambda. How about something like this:

private void assign(T)(T arg) /* rely on attribute inference */
{
    @trusted
    static void setField(T)(ref T field, const T value)
    {
        field = value;
    }

    static if (is(T : typeof(null)))
    { /* no alias this here */
        type_tag = JSON_TYPE.NULL;
    }
    else static if (is(T : string))
    {
        type_tag = JSON_TYPE.STRING;
        setField(store.str, arg); // allow attribute inference by evaluating alias this
                                  // separately from the mutation of the union member
    }
    else static if (isSomeString!T) // issue 15884
    {  /* no alias this here */
        type_tag = JSON_TYPE.STRING;
        // FIXME: std.array.array(Range) is not deduced as 'pure'
        () @trusted {
            import std.utf : byUTF;
            store.str = cast(immutable)(arg.byUTF!char.array);
        }();
    }
    else static if (is(T : bool))
    {  /* no need for setField because we're not mutating the union member */
        type_tag = arg ? JSON_TYPE.TRUE : JSON_TYPE.FALSE;
    }
    else static if (is(T : ulong) && isUnsigned!T)
    {
        type_tag = JSON_TYPE.UINTEGER;
        setField(store.uinteger, arg);
    }
    else static if (is(T : long))
    {
        type_tag = JSON_TYPE.INTEGER;
        setField(store.integer, arg);
    }
    else static if (isFloatingPoint!T)
    {
        type_tag = JSON_TYPE.FLOAT;
        setField(store.floating, arg);
    }
    else static if (is(T : Value[Key], Key, Value))
    {
        static assert(is(Key : string), "AA key must be string");
        type_tag = JSON_TYPE.OBJECT;
        static if (is(Value : JSONValue))
        {
            setField(store.object, arg);
        }
        else
        {
            JSONValue[string] aa;
            foreach (key, value; arg)
                aa[key] = JSONValue(value);

            setField(store.object, aa);
        }
    }
    else static if (isArray!T)
    {
        type_tag = JSON_TYPE.ARRAY;
        static if (is(ElementEncodingType!T : JSONValue))
        {
            setField(store.array, arg);
        }
        else
        {
            JSONValue[] new_arg = new JSONValue[arg.length];
            foreach (i, e; arg)
                new_arg[i] = JSONValue(e);

            setField(store.array, new_arg);
        }
    }
    else static if (is(T : JSONValue))
    {
        type_tag = arg.type;
        setField(store, arg.store);
    }
    else
    {
        static assert(false, text(`unable to convert type "`, T.stringof, `" to json`));
    }
}

Honestly, I would like to see a more elegant solution, but I can't think of a better one atm.

[PetarKirov]

Still unsafe, because the conversion from T to string might invoke a @system alias this-ed function.

[andralex]

Yeah, the right way is:

string t = arg;
() @trusted { store.str = t; }();

[andralex]

thx! this is a bit awkward but I'm glad we're going through it to clarify what kind of idioms we should improve and foster in the future.

[andralex]

Auto-merge toggled on