Re: [PATCH] Preserve order of dlopen'd modules in dll_list::topsort

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi David,

On Feb 25 16:27, David Allsopp wrote:
> This patch (below - I hope I have managed to format this email correctly)
> alters the behaviour of dll_list::topsort to preserve the order of dlopen'd
> units.
> 
> The load order of unrelated DLLs is reversed every time fork is called,
> since dll_list::topsort finds the tail of the list and then unwinds to
> reinsert items. My change takes advantage of what should be undefined
> behaviour in dll_list::populate_deps (ndeps non-zero and ndeps and deps not
> initialised) to allow the deps field to be initialised prior to the call and
> appended to, rather than overwritten.
> 
> All DLLs which have been dlopen'd have their deps list initialised with the
> list of all previously dlopen'd units. These extra dependencies mean that
> the unwind preserves the order of dlopen'd units.
> 
> The motivation for this is the FlexDLL linker used in OCaml. The FlexDLL
> linker allows a dlopen'd unit to refer to symbols in previously dlopen'd
> units and it resolves these symbols in DllMain before anything else has
> initialised (including the Cygwin DLL). This means that dependencies may
> exist between dlopen'd units (which the OCaml runtime system understands)
> but which Windows is unaware of. During fork, the process-level table which
> FlexDLL uses to get the symbol table of each DLL is copied over but because
> the load order of dlopen'd DLLs is reversed, it is possible for FlexDLL to
> attempt to access memory in the DLL before it has been loaded and hence it
> fails with an access violation. Because the list is reversed on each call to
> fork, it means that a subsequent call to fork puts the DLLs back into the
> correct order, hence "even" invocations of fork work!
> 
> An interesting side-effect is that this only occurs if the DLLs load at
> their preferred base address - if they have to be rebased, then FlexDLL
> works because at the time that the dependent unit is loaded out of order,
> there is still in memory the "dummy" DONT_RESOLVE_DLL_REFERENCES version of
> the dependency which, as it happens, will contain the correct symbol table
> in the data section. For my tests, this initially appeared to be an x86-only
> problem, but that was only because the two DLLs on x64 should have been
> rebased.
> 
> I'm very happy to include the complete detail for this and, for the
> extremely keen, the relevant Git branch in OCaml which demonstrates this
> problem. Given the way in which FlexDLL operates, I would contend that this
> is a sensible change of behaviour for the Cygwin DLL, though not a bug fix.
> I'd be extremely happy to see this patch integrated, as the workaround
> necessary in FlexDLL to support Cygwin's fork is horrible (and
> non-transparent to the library user).
> 
> This patch is licensed under 2-clause BSD as per winsup/CONTRIBUTORS,
> Copyright (c) 2017, MetaStack Solutions Ltd.

First of all, I think this makes perfect sense.  I just have a few
questions in terms of the patch itself.

- Your browser inserts undesired line breaks, so the patch is broken.
  Can you please resend the `git format-patch' output as attachment?

- While you're at it, please reformat your patch so the line length
  is not longer than 80 chars.

- Last but not least.  You add code to topsort so the loaded DLLs
  are handled first.  The subsequent code is untouched.  However,
  shouldn't the next loop then restrict calling populate_deps to the
  linked DLLs only, at least for performance?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature