bitwiseworks / libc

LIBC Next (kLIBC fork)
9 stars 4 forks source link

Replace __libc_gpfnDosOpenL and friends with DosOpenL #36

Closed dmik closed 5 years ago

dmik commented 5 years ago

We need to get rid from manually resolved DOSCALLS entry points like __libc_gpfnDosOpenL and use ones provided by LIBOS2.LIB which get replaced with high-memory safe wrappers when appropriate.

From https://github.com/bitwiseworks/sqlite-os2/issues/1#issuecomment-507436592:

I think I found out what's wrong. LIBC itself uses custom entry points for DosOpenL and some other calls (__libc_gpfnDosOpenL and friends) in some places including the fchown code path. These entry points are resolved at LIBC init time directly from DOSCALLS by DosQueryProcAddr. However, DosOpenL (as well as DosOpen and many others) is not high memory safe. And the string coming to this API is in fact in high memory (placed there by LIBC itself — when creating a new internal FH record for the file descriptor in open). Funny enough, LIBC provides safe wrappers for such calls, including DosOpenL. But given the direct resolve, this wrapper is simply not used.

I'm not sure why these direct resolves are still there — perhaps they are back from the days when LIBOS2.LIB lacked these "secret" DOSCALLS imports. Anyway, just replacing __libc_gpfnDosOpenL with DosOpen makes everything work as it should. Meaning that the R/O file is successfully reopened for writing indeed so that fchown succeeds.

I will try to reproduce it in a C test case to be 100% sure.

dmik commented 5 years ago

Fixed in the commit above. Note that no test case is really necessary because passing high-memory strings to DosOpenL is just not supported, period. JFYI, when I played around in a simple C test case using fchown, I saw DosOpenL not crashing but doing other weird things like returning error 2 (file not found) when being passed high-memory strings as file names.

Regarding the fix. Direct imports of these Large File Support APIs are not necessary now because these APIs are in LIBOS2 (as linker level imports from DOSCALLS) since long. And LIBC DLL is always linked against LIBOS2 anyway. But I left a flag (now named __libc_gfHaveLFS) used to check LFS presence across the LIBC code. This flag is initialized still by trying to import these APIs directly from DOSCALLS (note that the actual LIBC code will use LIBOS2 versions with their safe wrappers anyway — this resolving is only to detect LFS presence). However, given static linking of LIBC and LIBOS2, LIBC DLL will simply not load if these APIs are missing from DOSCALLS. So this flag is basically useless as it is always 1 if LIBC is loaded. But I decided to keep it in case if we ever want to resurrect spport of non-LFS systems in LIBC.

I will run the fresh build of LIBC locally and if it works fine, will cook a new RPM for it.

lerdmann commented 5 years ago

Even without an import library you could have added an import statement in the makefile... Provided that the entry point exists,of course. All the entry points are here: http://home.clara.net/orac/os2dll.htm also in the programming addendum.

dmik commented 5 years ago

Lars, this is already done (read about LIBOS2 above). Initially It was done that way so that LIBC could work on non-LFS systems, I believe.

lerdmann commented 5 years ago

I think you misunderstood my comment. What I meant is that you don't have to wait for an import lib to contain an entry point. You can always import an entry point "manually" via an import statement (if you know the DLL name and either the routine name or the ordinal). Anyways, it is now working as it should, which is good.

dmik commented 5 years ago

Sure I know that but this is not relevant in this case as LIBOS2 is part of LIBC. And what LIBOS2 does is a bunch of import statements. Makes no sense to add those statements to LIBC itself given that it's linked against LIBOS2. More over, in this particular case of DosOpenL we want to use a Safe wrapper (and it also lives in LIBOS2, not as an import statement of course but as a bit of assembly around an entry point).

dmik commented 5 years ago

It works quite fine in multiple test environments, this issue may be closed.