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

natdynlink reproducible segfault #4839

Closed
vicuna opened this issue Jul 22, 2009 · 8 comments
Closed

natdynlink reproducible segfault #4839

vicuna opened this issue Jul 22, 2009 · 8 comments

Comments

@vicuna
Copy link

vicuna commented Jul 22, 2009

Original bug ID: 4839
Reporter: furuse
Assigned to: @mshinwell
Status: resolved (set by @mshinwell on 2017-06-09T15:17:38Z)
Resolution: duplicate
Priority: normal
Severity: crash
Version: 3.11.1
Target version: later
Category: otherlibs
Related to: #4229 #4231 #6462 #6950 #6957
Monitored by: @ivg @rixed @glondu

Bug description

A packed module in a dll cmxs overwrites the packed module of the same name in the host program at dynlinking. If these two packed modules have incompatible signatures, the program may easily seg-faults.

We have found this when we wrongly created dll cmxs linking with a module of the host program. I upload a reproducible example. make all test shows the crash.

  • plugin : a base dll which works fine
  • plugin2 : a dll with a incompatible signature. Rejected correctly at linking.
  • plugin3 : same as plugin2, but mistakenly linked with the api module. Self-contained. Crashes the host program at linking.
  • plugin4 : a dll which shows that the overwrite. The host programs Packed.Api.zero is overwritten by the dll's Packed.Api.zero.

File attachments

@vicuna
Copy link
Author

vicuna commented Jul 22, 2009

Comment author: @alainfrisch

I reported such a problem some time ago: #4229. In fact, you don't even need to use dynlink in order to break the type system when you link in different modules with the same name/signature; regular packing/linking will do: #4231.

@vicuna
Copy link
Author

vicuna commented Nov 27, 2015

Comment author: @alainfrisch

Turning to later: no clear resolution plan, and bug present for years.

@vicuna
Copy link
Author

vicuna commented Nov 28, 2015

Comment author: @xavierleroy

Just refusing to dynlink a compilation unit A if the main program already contains a unit named A would go a long way towards fixing this group of PRs.

@vicuna
Copy link
Author

vicuna commented Jan 15, 2016

Comment author: @ivg

I've uploaded two solutions, the first one is to bail out if someone tries to reinitialize a unit that is already in the main program. (See fail.patch)

Another approach, that is less conservative, and requires some consideration is to silently ignore such units and keep moving. (See ignore.patch).

I would advocate for the latter solution. First of all it looks safe, as we check the module that we ignore for a consistency with that one, that is already loaded. Second, it has intuitive behavior, i.e., it is ok to link two programs, that imports the same library. Third ... if we will bail out, then it may break plugin solutions for BAP, Frama-C, and maybe other projects. But honestly speaking, I still can't proof to myself, that this is a safe solution.

As far as I understand, the root of the problem here is that whenever we create a plugin that references some library as cmx or cmxa then the definitions (ui_defines) from the referenced library are copied to the dynu_defines field of the plugin. As a result the code is included, instead of being imported. So, ideal solution would be to never include other libraries in the plugin (do not use -linkpkg in ocamlfind's parlance). Then we will always work with a thin plugin, i.e., a shared library that never includes any other libraries, but only imports them. And at load time, we should satisfy all imports, that are not yet linked into the host program.

P.S. I'm not sure what Xavier meant by "refuse". To raise an error or to silently ignore?

@vicuna
Copy link
Author

vicuna commented Jan 15, 2016

Comment author: @ivg

I've upload a safe.patch that looks more safe for me than the ignore.patch, as it explicitly checks for all global names. This should also work for packaged objects.

@vicuna
Copy link
Author

vicuna commented Jan 18, 2016

Comment author: @glondu

So, ideal solution would be to never include other libraries in the
plugin (do not use -linkpkg in ocamlfind's parlance). Then we will
always work with a thin plugin, i.e., a shared library that never
includes any other libraries, but only imports them. And at load time,
we should satisfy all imports, that are not yet linked into the host
program.

This is what should be done in my humble opinion.

In Ocsigen, we create a Findlib package per plugin, and use Findlib in the host program to figure out what cmxs need to be loaded at dynlink time.

@vicuna
Copy link
Author

vicuna commented Jan 18, 2016

Comment author: @ivg

In Ocsigen, we create a Findlib package per plugin, and use Findlib in the host program to
figure out what cmxs need to be loaded at dynlink time.

Yep, we're using the same approach. But findlib even with its new dynload module, that records already loaded dependencies and predicates, still can't protect you fully from a corruption of state and segfaults. The problem is that findlib doesn't have an access to an internal caml_plugin_header data, so it can track dependencies only on its own level of abstraction, that not necessary reflects the real situation. For example, oasis will break this abstractions and link internal libraries without using -package option.

@vicuna
Copy link
Author

vicuna commented Jun 9, 2017

Comment author: @mshinwell

This has been fixed by #1063 - please continue any discussion there.

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

No branches or pull requests

2 participants