From: Jinoh Kang Subject: Re: [PATCH v3 07/13] loader: Don't clobber existing memory mappings when reserving addresses. Message-Id: Date: Thu, 27 Jan 2022 01:50:45 +0900 In-Reply-To: <87tudq7to1.fsf@wine> References: <87y233yfx3.fsf@wine> <8bbcb555-b498-45d4-2150-692dea843ce9@gmail.com> <87tudq7to1.fsf@wine> On 1/26/22 18:06, Alexandre Julliard wrote: > Jinoh Kang writes: > >> On 1/26/22 00:48, Alexandre Julliard wrote: >>> Jinoh Kang 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