Session storage returns garbage

[mingodad]

Hello !

With this sample we can see that afer some calls we start getting garbage on some of the session member variabls (peer):

Execute this server and browse to "http://127.0.0.1:8086/login" after that browse "http://127.0.0.1:8086/" and we will see on console (check_auth : / : 127.0.0.1 : 127.0.0.1 : 1 : DAD)
then run the "ab" command and again browse "http://127.0.0.1:8086/" and probably we will get this(check_auth : / : ��������� : 127.0.0.1 : 1 : DAD)

import vibe.appmain;
import vibe.http.server;
import vibe.core.log;
import std.datetime;

struct UserSession {
    bool loggedIn = false;
    bool isAdmin = false;
    string userName;
    string host;
    ushort port;
    string peer;
    bool viewOnly;
    string authorization_code;
    string access_token;
    DateTime dtime;
}

void handleRequest(HTTPServerRequest req, HTTPServerResponse res)
{
    if (req.path == "/")
    {
        if (!req.session) req.session = res.startSession();
        UserSession usession = req.session.get!UserSession("login");
        logInfo("check_auth : %s : %s : %s : %d : %s", req.path, usession.peer, req.peer, usession.loggedIn, usession.userName);
        res.writeBody("Hello, World!", "text/plain");
    } else if(req.path == "/login") {
        UserSession usession;
        usession.loggedIn = true;
        usession.userName = "DAD";
        usession.peer = req.peer;
        if (!req.session) req.session = res.startSession();
        req.session.set("login", usession);
        res.writeBody("Hello, World! Loged In !", "text/plain");
    }
}

shared static this()
{
    auto settings = new HTTPServerSettings;
    //settings.options |= HTTPServerOption.distribute;
    settings.port = 8086;
    settings.bindAddresses = ["::1", "127.0.0.1"];
    settings.sessionStore = new MemorySessionStore;
    listenHTTP(settings, &handleRequest);
}

ab command:
ab -n 5000 -c 1000 -k http://127.0.0.1:8086/

[mingodad]

My friend helped me to narrow down the problem somehow, it seems that the problem is that the HTTPServerRequest.peer having it's origin from a range of a char[64] variable is the problem, the range remain pointing to the memory but the memory content changes.

Windows also use the same way to return the peer string.

Probably more usages like this need to be revised on other parts of the code.

!!!! WATCH OUT TO RETURNING RANGES !!!!

package final class Libevent2TCPConnection : TCPConnection {
    private {
        bool m_timeout_triggered;
        TCPContext* m_ctx;
        string m_peerAddress;
        ubyte[64] m_peekBuffer;
        bool m_tcpNoDelay = false;
        bool m_tcpKeepAlive = false;
        Duration m_readTimeout;
        char[64] m_peerAddressBuf;
        NetworkAddress m_localAddress, m_remoteAddress;
    }

    this(TCPContext* ctx)
    {
        m_ctx = ctx;

        assert(!amOwner());

        m_localAddress = ctx.local_addr;
        m_remoteAddress = ctx.remote_addr;

        void* ptr;
        if( ctx.remote_addr.family == AF_INET ) ptr = &ctx.remote_addr.sockAddrInet4.sin_addr;
        else ptr = &ctx.remote_addr.sockAddrInet6.sin6_addr;
        evutil_inet_ntop(ctx.remote_addr.family, ptr, m_peerAddressBuf.ptr, m_peerAddressBuf.length);
        m_peerAddress = (cast(string)m_peerAddressBuf[0 .. m_peerAddressBuf.indexOf('\0')]).idup; //!!!!!!!! ().idup <<<<<<<<< here seems to solve the problem

        bufferevent_setwatermark(m_ctx.event, EV_WRITE, 4096, 65536);
        bufferevent_setwatermark(m_ctx.event, EV_READ, 0, 65536);
    }

[etcimon]

That's odd... why not just use m_peerAddress = m_remoteAddress.toAddressString() there?

[mingodad]

Good question ! I didn't tried to look further because it took me almost 2 days to found this and I was exausted. And also I di a search for ".." and there is usages like this on other places that can lead to unexpected behavior on some circusntances.

Cheers !

[etcimon]

Yes, I'd love to know if the native events (libasync) branch could also contain these errors. Thanks for the diagnostics and heads up!

[s-ludwig]

Yes, big thanks for tracking down the cause! Basically the issue that we have here is that the req, res parameters should in fact always be declared as scope, but early on I didn't think about that this could later be important for optimization.

The offending line of code is here where the Libevent2TCPConnection object is declared as a FreeListRef, which will get destroyed (intentionally) when the scope is left. I see two possible workarounds that don't introduce a GC memory allocation (idup):

• Make .peer a property that lazily calls idup, so that you only pay for what you need
• Create another char[64] m_peerBuffer; in HTTPServerRequest and copy the string into there.

My goal for the next version is to start deprecating non-scoped request handlers, so that the compiler can start to detect these kinds of bugs. Unfortunately DMD's enforcement of scope is currently extremely limited, so it will still miss a lot of cases.

[s-ludwig]

So to state this more explicitly, the reason the code works like that is an intentional optimization to avoid a GC allocation. For that reason, NetworkAddress.toAddressString also cannot be used here. But this is actually a very good reason to add an output range based overload of toAddressString...

[etcimon]

But this is actually a very good reason to add an output range based overload of toAddressString...

Or, maybe even prioritize lazy evaluation pretty much everywhere. The peer address, HTTP headers, JSON data... Only a few things are needed (URL, compression) when moving into the request handler and otherwise, if the storage of individual elements can be avoided I also assume it will speed up the typical static page delivery. I think there's also a cost associated to zeroing the dictionary list for 64 key/value pointers on every request (only for the headers), so maybe a single string allocation for the entire headers would do. Same for the forms and files.

[etcimon]

Also another possible optimization, the FixedRingBuffer could avoid the zeroing of its data segments because it has internal indexing control...

[s-ludwig]

The original issue is fixed when using vibe-core. The req.peer property is allocated on-demand there. Alternatively, req.clientAddress.toAddressString() can be used with a sink callback, to enable getting the address string without allocations.

[s-ludwig]

The HTTP improvements/optimizations will be part of the upcoming HTTP package rework.