trilogy-libraries / trilogy

Trilogy is a client library for MySQL-compatible database servers, designed for performance, flexibility, and ease of embedding.
MIT License
700 stars 69 forks source link

[Ruby] Potential compaction issue in `rb_trilogy_connect`? #140

Open casperisfine opened 10 months ago

casperisfine commented 10 months ago

While working on https://github.com/trilogy-libraries/trilogy/pull/139 I noticed something suspicious.

We have a number of statements like this:

    if ((val = rb_hash_lookup(opts, ID2SYM(id_host))) != Qnil) {
        Check_Type(val, T_STRING);

        connopt.hostname = StringValueCStr(val);

opts is held as @connection_options on the Trilogy instance, so val and it's char * won't be GCed, however:

casperisfine commented 10 months ago

Ok, so down the road, Trilogy will call strdup on these char *, so if there is a compaction issue, it's only during initialize. So you'd need GC.auto_compact = true and GC to trigger inside rb_trilogy_connect but before try_connect, which seem unlikely.

I'll double check, but it's probably a red herring.

casperisfine commented 10 months ago

Alright, I think the only risk is the two handle_trilogy_error calls after try_connect:

    int rc = try_connect(ctx, &handshake, &connopt);
    if (rc == TRILOGY_TIMEOUT) {
        rb_raise(Trilogy_TimeoutError, "trilogy_connect_recv");
    }
    if (rc != TRILOGY_OK) {
        if (connopt.path) {
            handle_trilogy_error(ctx, rc, "trilogy_connect - unable to connect to %s", connopt.path);
        } else {
            handle_trilogy_error(ctx, rc, "trilogy_connect - unable to connect to %s:%hu", connopt.hostname,
                                 connopt.port);
        }
    }

AFAICT try_connect could alloc and trigger GC, causing connopt.path and connopt.hostname to be potentially pointing at garbage.