186599 - PR_LoadLibrary() doesn't support relative paths.

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Doug Turner (:dougt)]

I ask ccarlen to look at the failure of xpcom glue on OSX.  Apparently, the
nspr's PR_LoadLibrary function doesn't handle relative paths correctly.  


ConradCarlen: heh - all this should take to fix is one call to realpath(3)

Comment 1

[Wan-Teh Chang]

Could you give me a test case or point out where
the error is in the source code?  Thanks.

Comment 2

[Chris McAfee]

xpcom/sample/nsTestSample, this fails on OSX but works on Linux, presumeably win32.

Comment 3

[Conrad Carlen (not reading bugmail)]

> or point out where the error is in the source code?
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/linking/prlink.c#763
Apparently, FSPathMakeRef() requires a full path. If the "name" param is made to
be a full path by realpath(3), it works.

There's more to this problem. We end up trying to use
NSCreateObjectFileImageFromFile to load the lib. That won't work on a lib linked
without the -bundle flag, as libxpcom.dylib is :-/

Comment 4

[Wan-Teh Chang]

Doug,

You will need to build a version of the xpcom library
with the -bundle flag.  Only that version (which would
most likely be called libxpcom.bundle) can be
dynamically loaded. On Mac OS X a dynamic shared library
(.dylib) and a loadable bundle (.bundle) are different
things while on Unix and Windows .so and .dll can be
used in both ways.

Comment 5

[Doug Turner (:dougt)]

do we really need two versions?  (Why would i ever want a shared library that
can't be programmatically loaded?)

Comment 6

[Wan-Teh Chang]

Doug, this is one peculiarity of Mac OS X (or rather Darwin).

libxpcom.dylib is a shared library that you can specify at
link time with the -lxpcom flag.  Obviously shared libraries
used in this way are still useful.

libxpcom.bundle is a shared library that you can load at
run time with the equivalent of the dlopen() function.

I asked your first question on [e-mail] before
and the answer I got was yes, we really need two versions.

Comment 7

[Doug Turner (:dougt)]

I wonder what the reasons were....  also, maybe the way I am attempting the
support of the GRE is incorrect on OSX.

do we need to do this same dual library trick for other libraries while we are
touching stuff?  

Comment 8

[hacker formerly known as [e-mail]]

Attachment: Create libxpcom.bundle

Comment 9

[Chris McAfee]

Comment on attachment 110043 [details] [diff] [review]
Create libxpcom.bundle

patch looks good to me, r=mcafee

Comment 10

[Doug Turner (:dougt)]

chris, can you/i check this patch in?  

Comment 11

[Doug Turner (:dougt)]

(I updated my other patches to look for a libxpcom.bundle instead of libxpcom.so
on macosx)

Comment 12

[hacker formerly known as [e-mail]]

The patch has been checked in.

Comment 13

[Conrad Carlen (not reading bugmail)]

Doug - is the original problem reported, which the patch doesn't address, still
an issue?

The header doc for PR_LoadLibrary doesn't specify that the given path can be
relative or full. Not supporting relative paths was a problem for a test app.
Since we obviously use full paths everywhere else, should this just be fixed in
the test app?

Comment 14

[Doug Turner (:dougt)]

i do not know how loading libraries (or bundles) on OS X works.  However, I want
to be able to do something like this:

PR_LoadLibary("./foo.bundle");

Is that supported?

Comment 15

[Wan-Teh Chang]

PR_LoadLibrary should support relative paths.

Comment 16

[Vidar Haarr (not reading bugmail)]

So this is fixed then ?

Comment 17

[Doug Turner (:dougt)]

no.  it is still broken.  conrad is going to look at it shortly.

Comment 18

[Conrad Carlen (not reading bugmail)]

Attachment: patch allow relative paths, etc.

diff lost its mind on this one - the changes aren't as strange as they look.

Basically, this breaks the different methods that can be used to load libs on
the mac into subroutines, and calls those in a certain order depending on what
type of runtime arch we are - the format that is most native is tried first.
This fixes the performance loss reported in bug 187185. Relative paths now
work. For Mach-O, this is because the new code goes straight to
NSCreateObjectFileImageFromFile(), which can work with a relative path. Before,
it used to fail on relative paths because of some Mac file mgr calls which that
were done for the benefit of CFM library loading calls.

Comment 19

[Simon Fraser [no longer active]]

Could you attach the entire file, for ease of viewing? Thanks.

Comment 20

[Conrad Carlen (not reading bugmail)]

Attachment: complete prlink.c

Here is the complete file. If you apply the patch to a copy, CodeWarrior gives
a much clearer diff.

Comment 21

[Simon Fraser [no longer active]]

Comments:
> /*
>  * macLibraryLoadProc is a function definition for a Mac shared library
>  * loading method. The "name" param is the same full or partial pathname
>  * that was passed to pr_LoadLibraryByPathName. The function must fill
>  * in the fields of "lm" which apply to its library type as well as the
>  * "name" field. Returns 0 if successful.
>  */
> 
> typedef PRInt32 (*macLibraryLoadProc)(const char *name, PRLibrary *lm);

I think this should return an OSErr, or, if it's too hard to unify different
error spaces, just a boolean.

> /*
>  ** Returns a CFBundleRef if the file name refers to a Mac OS X bundle directory.
>  ** The caller is responsible for calling CFRelease() to deallocate.
>  */

Comments looks incorrect. This returns errors, not CFBundleRefs.

> #if TARGET_CARBON
> static PRInt32 loadBundle(const char *name, PRLibrary *lm)
> {
>     CFURLRef bundleURL;
>     CFBundleRef bundle = NULL;
>     
> #ifdef XP_MACOSX
>     char pathBuf[PATH_MAX];
>     const char *resolvedPath;
>     CFStringRef pathRef;
> 
>     /* Takes care of relative paths and symlinks */
>     resolvedPath = realpath(name, pathBuf);
>     if (!resolvedPath)
>         return errno;

If you decide to return OSErrs, need to convert errno to an OSErr.

> #if defined(XP_MAC) || defined(XP_MACOSX)
>     {
>     int					  i;
>     OSErr                 err;
> 
>     macLibraryLoadProc loadProcs[] = {
> #if defined(XP_MACOSX)
>         loadViaDyld, loadBundle, loadViaCFM
> #elif TARGET_CARBON
>         loadViaCFM, loadBundle
> #else
>         loadViaCFM
> #endif
>     };

Make loadProcs const and static?

> #elif defined(USE_MACH_DYLD)
>     NSObjectFileImage ofi;
>     NSModule h = NULL;
>     if (NSCreateObjectFileImageFromFile(name, &ofi)
>             == NSObjectFileImageSuccess) {
>         h = NSLinkModule(ofi, name, NSLINKMODULE_OPTION_PRIVATE);
>     }

Do we need this stuff any more? It's confusing to have this code
appear twice.

> #ifdef XP_UNIX
> #ifdef HAVE_DLL
> #ifdef USE_DLFCN
>     result = dlclose(lib->dlh);
> #elif defined(USE_HPSHL)
>     result = shl_unload(lib->dlh);
> #elif defined(USE_MACH_DYLD)
>     result = NSUnLinkModule(lib->dlh, FALSE);
> #else
> #error Configuration error
> #endif
> #endif /* HAVE_DLL */
> #endif /* XP_UNIX */
> #ifdef XP_PC
>     if (lib->dlh) {
>         FreeLibrary((HINSTANCE)(lib->dlh));
>         lib->dlh = (HINSTANCE)NULL;
>     }
> #endif  /* XP_PC */
> 
> #if defined(XP_MAC) || defined(XP_MACOSX)
>     /* Close the connection */
>     if (lib->connection)
>         CloseConnection(&(lib->connection));
> #if TARGET_CARBON
>     if (lib->bundle)
>         CFRelease(lib->bundle);
> #endif
> #if defined(XP_MACOSX)
>     if (lib->wrappers)
>         CFRelease(lib->wrappers);
> #endif
> #endif
> 

It's unfortunate that the unloading code doesn't use the same
#ifdef layout as the loading code. Uloading Mac OS X dylibs
falls under #ifdef UNIX && defined(USE_MACH_DYLD), which is
rather confusing. I think this should be fixed.

Comment 22

[Conrad Carlen (not reading bugmail)]

Simon - I agree and will post a new patch.

typedef PRInt32 (*macLibraryLoadProc)(const char *name, PRLibrary *lm);
I'll make it return PRBool because of the various error code spaces.

USE_MACH_DYLD, outside of XP_MACOSX, does lead to some ugliness :-/

This still exists separately for the sake of the NextStep build of NSPR. If that
isn't a supported build, things defined for USE_MACH_DYLD could be pulled inside
XP_MACOSX. I assumed that we wanted the NextStep build to still work. Anybody
know its status?

Comment 23

[Wan-Teh Chang]

USE_MACH_DYLD (i.e., standalone Darwin) should continue to
work.

Comment 24

[Conrad Carlen (not reading bugmail)]

Attachment: patch v2

Changed the return type of (*macLibraryLoadProc)() to PRBool for
success/failure.
Made macLibraryLoadProc loadProcs[] = ... static const
Replaced some #ifdef DEBUG printfs in Mac code with PR_LOG

Most of the XP_MACOSX work in this file is done with USE_MACH_DYLD. There are
now two occurances of NSCreateObjectFileImageFromFile(), but I can't see a way
of fixing that. Anyway, it's better than the existing goto which allows us to
fall through to some USE_MACH_DYLD code below. I think the only way this could
really be cleaned up is to break it all out into md code.

Comment 25

[Conrad Carlen (not reading bugmail)]

Comment on attachment 110843 [details] [diff] [review]
patch allow relative paths, etc.

I wish when you obsoleted a patch, requests were canceled automatically.

Comment 26

[Wan-Teh Chang]

Comment on attachment 111019 [details] [diff] [review]
patch v2

This patch assumes that TARGET_RT_MAC_CFM is 1
if XP_MAC is defined.  Is that a valid assumption?

Comment 27

[Conrad Carlen (not reading bugmail)]

Yes, looking at <TargetConditionals.h>, and given how we define XP_MAC and
XP_MACOSX.

Comment 28

[Wan-Teh Chang]

Attachment: patch v3

Your patch is very good, Conrad.  Thank you.
I just did some editing to follow the NSPR
coding style.

1. I added the pr_ prefix to the new static
functions.

2. I did some formatting of the comment blocks
and broke long lines into lines shorter than
80 columns.

3. I changed the macLibraryLoadProc function
to return PRStatus instead of PRBool and not
to set lm->name.  lm->name is now set by
pr_LoadLibraryByPathName.

4. I added a new static function for the common
code (NSCreateObjectFileImageFromFile) between
XP_MACOSX and USE_MACH_DYLD.

Comment 29

[Wan-Teh Chang]

I've checked in the patch on the NSPR trunk and
NSPRPUB_PRE_4_2_CLIENT_BRANCH.

Doug, could you verify that PR_LoadLibrary now
supports relative paths?