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

jl_dlsym incorrectly assumes a NULL return is an error #28881

Closed
Keno opened this issue Aug 24, 2018 · 5 comments
Closed

jl_dlsym incorrectly assumes a NULL return is an error #28881

Keno opened this issue Aug 24, 2018 · 5 comments

Comments

@Keno
Copy link
Member

Keno commented Aug 24, 2018

Quoth the man page:

Since the value of the symbol could actually be NULL (so that a NULL return from dlsym() need not indicate an error), the correct way to test for an error is to call dlerror() to clear any old error conditions, then call dlsym(), and then call dlerror() again, saving its return value into a variable, and check whether this saved value is not NULL.

NULL-mapped symbols are sometimes used to indicate feature levels (e.g. in libstdc++). We should make sure to support them. One complication is that we exposed this behavior through dlsym_e. We should probably have a function that returns either a pointer value or nothing if the symbol was not found.

@staticfloat
Copy link
Sponsor Member

So as it stands now, jl_dlsym_e() does not disambiguate between failure (symbol does not exist) and success with a null result (symbol exists but points to the memory location 0x00000000). In order for this to work, we either need to, in C code, come up with a new value that means "symbol was not found" (perhaps change that to be (int_ptr)(-1) instead of NULL?), or change the semantics of jl_dlsym_e to use sidechannel outputs (e.g. bool jl_dlsym_e(void * handle, const char * symbol, void ** symbol_addr) or somesuch).

What do you guys think? Either way it's a breaking change, so let's do The Right Thing (TM). I personally like the sidechannel API look better.

@staticfloat
Copy link
Sponsor Member

Also, it looks like Windows does not support NULL-valued symbols: https://msdn.microsoft.com/en-us/library/windows/desktop/ms683212(v=vs.85).aspx

@Keno
Copy link
Member Author

Keno commented Aug 24, 2018

int_ptr)(-1)

Also a valid symbol.

We need an extra bit of information, which is why I suggested using Union{Ptr, Nothing} above. I think we can add a new API that does that and reimplement the two existing ones on top of that. dlsym is a bit easier since adding a null result is non breaking, but would probably have to deprecate dlsym_e in favor of a new API with the extra bit of information.

@staticfloat
Copy link
Sponsor Member

I think we need to first fix this at the C level, not the Julia level. Unless you’re suggesting using Julia’s C API to return a Union from jl_dlsym_e()?

@Keno
Copy link
Member Author

Keno commented Aug 24, 2018

I'm not, but we can do whatever at the C level (your suggestion is fine), we don't have stability guarantees there.

staticfloat added a commit that referenced this issue Aug 24, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 24, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 26, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 26, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 26, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 27, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 27, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 28, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
staticfloat added a commit that referenced this issue Aug 29, 2018
* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`.

* Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.
vtjnash added a commit that referenced this issue Mar 21, 2019
missed as part of #28953, c.f. #28881
vtjnash added a commit that referenced this issue Mar 21, 2019
missed as part of #28953, c.f. #28881
vtjnash added a commit that referenced this issue Mar 21, 2019
missed as part of #28953, c.f. #28881
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

2 participants