net.WriteTable stack-overflow flaw

[Acecool]

While looking at: http://facepunch.com/showthread.php?t=1412399 I noticed there appeared to be a net.WriteTable stack-overflow bug whereby if you nest a parent table to a child, it'll continue trying to send. While it may be up to the developer to ensure stuff like this doesn't happen, what is going to stop someone on a CL scripts allowed server from spamming a server with WriteTables?

Code to produce issue:

concommand.Add( "dev_net_overflow", function( _p, _cmd, _args )
    local _tab = {
        test = "test";
        [ 1 ] = 123;
        test2 = "testing";
    };
    _tab.tab = _tab;
    _tab.tabby = "right";

    print( tostring( _tab ) );
    -- print( _tab );

    net.Start( "TestNetWriteTableOverFlow" );
        net.WriteTable( _tab );
    net.Broadcast( );
end );

where: _tab.tab = _tab; is the issue.

I started writing a fix in Lua, but the way the net-functions are called and re-called for each bit of data, and having no return statement to piggyback a list of processed tables, it may be better to have net.Start set up a table of processed data, and if it hits a table do the following logic:

// Call the sent_data, or processed_tables in net.Start, reset it there and it shouldn't need to be put in net.Send/Broadcast...
// Also, by setting sent_data out here, the main table will always get sent through but without it being reset it could prevent legitimate nested-table-data from being sent if it has been sent before which is why it needs to be in net.Start... I could write a quick override, but for official fixes it should be implemented a little nicer.
local sent_data = { };
function net.WriteTable( tab )

    for k, v in pairs( tab ) do

        // If the data snippet is a table, lets test for stack-overflow
        if ( istable( v ) ) then
            // Make sure we never repeat the primary / parent table
            sent_data[ tostring( tab ) ] = true;

            // Check to see if the current table has already been sent
            local _key = tostring( v );
            if ( sent_data[ _key ] ) then

                // The table has already been sent, change it from writing a table to writing a string of the table identifier/reference to help user debug
                v = _key;

                // Outputting error to let them know they messed up
                Error( "Preventing stack-overflow, do not nest a table inside of itself.\n" );

                // Continue, or not, If we write a string then no need, if we don't then continue...
                -- continue;
            end

            // Update the list of processed tables, so next time it tries this one, prevent it..
            sent_data[ _key ] = true;

        end

        net.WriteType( k )
        net.WriteType( v )

    end

    -- End of table
    net.WriteUInt( 0, 8 )

end

As said, sent_data / processed_tables is better suited to be re/initialized in net.Start because of the way net.WriteTable is written. This is a minor fix in terms of the dev should know what data is being sent and shouldn't ever nest a parent table in a child.

In terms of security against a type of attack, this may be a good idea to implement...

[Lexicality]

There's no attack - net.WriteTable on the client overflows and kills the frame before anything is sent to the server. Also, this is no worse than running

net.Start("Hi");
net.WriteString(string.rep("a", 64000))
net.SendToServer()

in a think loop to saturate the client's upstream channel in an attempt to make the server do a little more work than normal (would this even have any effect? The 64KBps clamp should prevent network flooding and the server will just drop messages with no reciever)

[Acecool]

True; but while not all things can be fixed, at least some things could be resolved ( such as the table going through since it is so easy to tostring( table ) and use that identifier as a key to an array, check to see if it was sent during that net.Start; if not then parse, otherwise send as string reference )