[PATCH v3 07/13] loader: Don't clobber existing memory mappings when reserving addresses.

[Jinoh Kang]

On 1/26/22 18:06, Alexandre Julliard wrote:
> Jinoh Kang <jinoh.kang.kr at gmail.com> writes:
> 
>> On 1/26/22 00:48, Alexandre Julliard wrote:
>>> Jinoh Kang <jinoh.kang.kr at gmail.com> writes:
>>>
>>>> Today, the preloader makes no attempt to avoid unmapping existing
>>>> memory mappings except the initial stack.  This results in irrevocably
>>>> unmapping some useful preallocated memory areas, such as vDSO.
>>>>
>>>> Fix this by reading /proc/self/maps for existing VMAs, and splitting
>>>> mmap() calls to avoid erasing existing memory mappings.
>>>
>>> That defeats the purpose of using the preloader.
>>
>> The intention was to *incrementally* scrape memory areas for the reserved ranges,
>> relocating any critical areas (vDSO, stack, ...) along the way.
>>
>> It's also why this change is useless without the subsequent patches,
>> which calls map_reserve_preload_ranges again to actually fill out all
>> the gaps previously occupied by vDSO/stack.
> 
> I don't see the point. If you want to remap vDSO you can do that first,
> and then reserve the full range. You don't need all that complexity.

If we remap vDSO _without_ reserving memory first, then the kernel may
end up choosing a reserved address for the new vDSO address, especially
on 32-bit.

We can implement the address allocation algorithm from scratch avoiding
the preload area explicitly ourselves, but I suppose it would result in
even more complexity.  This is also why we reserve those ranges in the
first place -- to let OS pick the address for us.

> 
> And as general advice for your patches, please try to avoid changing
> things that don't need changing, or adding infrastructure that isn't
> needed. It will make it easier to see the actual changes.
> 

My apologies for complicating the review work.  I'll try to separate such
patches into another serie, or omit them entirely if there's really no need
for them.

Perhaps there was something that I was thinking wrong about along the way.
In that case, I'll try my best to explain my motivation and/or assumption behind
those patches.  I'd like to not repeat the same mistake again, so I'd be happy to
learn about what went wrong (so that I can correct it).  Please feel free to
ignore them if you wish, though.

- loader: Use long instead of int for syscall return type in i386 code.

  I was planning to add more syscalls, and found that i386 code used "int" for
  syscall returns. My assumption was that it was a style issue (presumably
  32-bit-area Wine's leftover) that I was expected to address _before_ I could
  add more inline asm blocks.

  [The reason why I found "int" confusing was that I usually read "long" as
   "machine register-width integer," and "int" as "integer that either only
   needs to hold small values (e.g. exit code) _or_ has to be 32-bit for some
   reason."  Granted int = long on ILP32, but someone looking at the
   code might not be able to immediately verify that the type is correct unless
   they also consider that "int $0x80" is for the i386 ABI (or that the code region
   is guarded by ifdef __i386__), and i386 is a 32-bit architecture.
   I supposed that would be extra cognitive load for someone reviewing/auditing
   the code.]

  Perhaps my mistake here was one of the following:
  1. The code style wasn't an issue at all. I (incorrectly) assumed that
     the code style was a general consensus among systems programmers.
  2. The code style was indeed a problem, but it wasn't really required to
     fix them in the *same* patch serie.
  3. The style fix was required, but it came too early in the serie.

- loader: Remove GCC <=4.x EBX register spilling workaround for i386.

  Similar to above, but debugging related.  Again I was adding asm blocks
  around, so I felt it obliged to fix it before adding more code.

- loader: Enable dumping additional vectors in dump_auxiliary.

  This is not a critical patch, but merely a debugging aid.  Since it was
  obvious from the subject, I supposed it wouldn't hurt to include it
  towards the end of the series.
  I agree it would certainly have been better off as a separate patch, however.


Meanwhile, following are patches that I deem necessary and not gratuitous:

- loader: Refactor argv/envp/auxv management.

  This was necessary because I had to pass around the pointers to
  stack objects a lot.  Examples include getenv() for letting
  user control remapping behaviour via env vars, and stack relocation
  (which requires shifting all the argv/envp/auxv pointers).
  If the pointer variables were not in one place, the latter would
  make the code a lot hairy (by passing around all the scattered
  stack pointers) and unmaintainable.

- loader: Refactor number parsing to own function.

  Number parsing was only for WINEPRELOADRESERVE.  Now I need it for
  parsing /proc/self/maps as well.


I also looked through all of my past patchsets, and here's my thoughts about them:

- kernel32/tests: Test module refcounting with forwarded exports.
  => You have pointed out that the IATGAS hack is ugly (which is true).
     Perhaps this one counts as an "unnecessary infrastructure."

     As for the justification of a new DLL (forward4), it was particularly
     hard to find a concrete example that exactly suits the tests' needs.

     1. The DLL has to forward calls to another DLL *but* must not actually
        import from it.  This excludes DLLs like kernel32 which both forwards to
        *and* imports from kernelbase.
     2. The above condition must hold for all Windows versions being tested *and*
        in Wine.
     3. The DLL must not have been already loaded (either directly or indirectly via
        dependencies) in the process.  This excludes DLLs like kernel32, advapi,
        user32, gdi32, shell32, setupapi, userenv, and etc.

     The most recent iteration of this patchset uses ICMP/IPHLPAPI, but I'm still
     not 100% sure about the compatibility issue.
     An alternative would be to keep forward4 and modify winebuild so that TESTDLLs
     can import from other TESTDLLs, but it seemed like an overkill.

- winedbg(gdbproxy): don't misbehave on module names w/ special characters; modernise
  => Patchset does contain a lot of refactoring work, but I tried to justify them
     in the commit message body to my best.

- sock_recv() fix series
  => I attempted to minimize extra changes and/or infrastructure, but it might not have been perfect.
     In case some non-obvious change was needed, I tried to justify them in the patch message body.
     Perhaps there are other ways to implement them I couldn't think of.

Meanwhile following patches are ostensibly gratuitous in nature (it might or might not be useful; it's up to your decision):

- winedbg: use heap allocation for module filenames in handle_debug_event.
  => May seem gratuitous, but fixes module filenames being truncated over MAX_PATH.
- server: Allow skipping debug handle retrieval in get_process_debug_info.
  => Functionality may be gratuitous, but may closely match Windows NT kernel more.
     Practially untestable (Wine lacks needed infrastructure) for now, so up to the maintainer's decision.
- ntdll: Implement NtSetInformationVirtualMemory.
  => Functionality may be gratuitous or maybe some apps need it, up to the maintainer's decision.
- ntdll: Implement __fastfail().
  => Functionality may be gratuitous or maybe some apps need it, up to the maintainer's decision.
- ntdll: Make syscall dispatcher properly restore X16 and X17 in ARM64.
  => Completely up to the maintainer's decision.
- ws2_32/tests: Add order-agnostic check in test_simultaneous_async_recv.
  => Completely up to the maintainer's decision.

Again, thanks a lot!

-- 
Sincerely,
Jinoh Kang