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

RFC: Add epoxy_set_resolver_failure_handler() #131

Merged
merged 1 commit into from Sep 11, 2017

Conversation

nwnk
Copy link
Collaborator

@nwnk nwnk commented Jul 5, 2017

I'd like to use epoxy in xserver's libglx, but the fatal error on resolve
failure makes this a little awkward. My generated glx protocol code is
going to be wired up regardless of the gl extensions the renderer actually
supports, and while a well-behaved client isn't going to send a request
the connection doesn't support, I don't want a mischevious client to be
able to shoot down the entire X server. I could prevent this by mirroring
the same provider iteration walk in the generated code, but needing to do
that in two places instead of one kind of defeats the point of having
epoxy involved at all.

This change adds a failure handler that, if registered, is called instead
of abort. There's not much it can usefully do of course - I expect the
xserver handler will just chirp in the log and kill the client connection

  • so there's no closure pointer in the signature. I'm not totally sure the
    signal()-like "return the old handler" idiom is useful.

@ebassi
Copy link
Collaborator

ebassi commented Jul 6, 2017

I like the idea, and I'd probably use something like this in GTK+ as well, to ensure that the assertion goes through the GLib logging API.

@@ -92,6 +92,11 @@ EPOXY_PUBLIC bool epoxy_has_gl_extension(const char *extension);
EPOXY_PUBLIC bool epoxy_is_desktop_gl(void);
EPOXY_PUBLIC int epoxy_gl_version(void);

typedef void * (*epoxy_resolver_failure_handler_t)(const char *name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure why epoxy_resolver_failure_handler_t would need to return something.

I mean, if you plan on killing the connection then just calling this function and returning NULL from Epoxy itself would be enough; or you do plan on returning some other blob of data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have to return something else here.

"glFoo" is really a function pointer named epoxy_glFoo, which initially points to a function named epoxy_glFoo_global_rewrite_ptr. When it is first called, it calls epoxy_glFoo_resolver to GetProcAddress for the symbol, stores that value in epoxy_glFoo, then calls it. This way, on subsequent calls to glFoo, we don't have to look the symbol up again.

We're modifying the bottom of epoxy_glFoo_resolver here, so returning void isn't an option. Neither do I want to return NULL, as whatever function pointer the resolver returns is about to get called.

The X server code for this would look something like:

static void
ZapBadGlxClient(void)
{
    ErrorF("Killing broken GLX client %s\n", GetClientCmdName(currentClient));
    CloseDownClient(currentClient);
}

static void *
GlxResolverFailureHandler(const char *name)
{
    ErrorF("Tried to call unavailable GL function %s\n", name);
    return ZapBadGlxClient;
}

Unfortunately this means you only get the diagnostic about which unavailable function was being called on the first resolve failure for it, and subsequent abuse will just hit ZapBadGlxClient directly. I think I'm okay with that though, as I don't think jitting a stub function that remembers the name, nor generating dead function stubs in epoxy, would be worth the size or effort.

epoxy_resolver_failure_handler_t
epoxy_set_resolver_failure_handler(epoxy_resolver_failure_handler_t handler)
{
epoxy_resolver_failure_handler_t old = epoxy_resolver_failure_handler;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may need to be protected by a mutex, especially on Windows; or be tied to the TLS, like the GL context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the branch to fix this (also made the typedefs a bit more self-documenting)

Signed-off-by: Adam Jackson <ajax@redhat.com>
@nwnk nwnk force-pushed the nonfatal-resolve-handler branch from 013b8cb to 7ff061a Compare July 14, 2017 16:55
@nwnk
Copy link
Collaborator Author

nwnk commented Jul 27, 2017

Anything else needed for this?

@ebassi
Copy link
Collaborator

ebassi commented Aug 7, 2017

Looks good; I'm going to branch master for 1.4.* and merge this after the 1.5 bump, as it adds new API and behaviour.

@ebassi ebassi merged commit e9e0098 into anholt:master Sep 11, 2017
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

Successfully merging this pull request may close these issues.

None yet

2 participants