Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ruby] Potential compaction issue in rb_trilogy_connect? #140

Open
casperisfine opened this issue Dec 13, 2023 · 2 comments
Open

[Ruby] Potential compaction issue in rb_trilogy_connect? #140

casperisfine opened this issue Dec 13, 2023 · 2 comments

Comments

@casperisfine
Copy link
Contributor

While working on #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:

  • We don't ensure the string is frozen, so it could be mutated later on.
  • We these strings aren't pined, so if they are embedded, they could be moved elsewhere, causing connopt to point at garbage / another object.
@casperisfine
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant