The default bug view has changed. See this FAQ.

XPCOMGlueLoadXULFunctions should search library's symbol table

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jhp (no longer active), Assigned: jhp (no longer active))

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
PowerPC
Mac OS X
fixed1.8.0.2, fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nvn-dl])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
|XPCOMGlueLoadXULFunctions| on Mac OS X currently uses |NSLookupAndBindSymbol|, which searches for a symbol in the global symbol table (across all loaded libraries).  When using JavaXPCOM to embed XULRunner in Java, I get an error where |XPCOM_NATIVE(getComponentManager)| in nsJavaXPCOMGlue.cpp keeps calling itself, rather than calling the similarly named function in the XUL library.  This isn't a problem on other platforms, since they only look for the symbols in the given library (libXUL).  For Mac OS X, we should at least be using |NSLookupAndBindSymbolWithHint|, or better yet, |NSLookupSymbolInImage|.

Not sure why I didn't see this earlier; I'm guessing that in those cases, |NSLookupAndBindSymbol| was pulling the right symbol from the global symbol table, but today it's getting the wrong one.
Are the functions the same name? We typically make sure that the functions are named differently (e.g. NS_InitXPCOM2 in libxpcom.dylib forwards to NS_InitXPCOM_P in XUL). Though the InImage variant might also solve this issue.
(Assignee)

Comment 2

11 years ago
Yes, in my case, the functions are named the same.  Didn't think that would be a problem.  However, even if I did fix this, I still think we should fix this to only look in the XUL library, to prevent this issue from happening to others.  Working on a patch now.
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
Created attachment 210172 [details] [diff] [review]
patch
Attachment #210172 - Flags: review?(benjamin)
(Assignee)

Comment 4

11 years ago
Hmm, actually, we should probably be looking for |NS_GetFrozenFunctions| in the same way, right?
(Assignee)

Comment 5

11 years ago
Created attachment 210174 [details] [diff] [review]
patch v1.1

According to the docs, this should be faster, since we are only looking for the images in the appropriate libs, rather than looking in the global symbol table.
Attachment #210172 - Attachment is obsolete: true
Attachment #210174 - Flags: review?(benjamin)
Attachment #210172 - Flags: review?(benjamin)
Comment on attachment 210174 [details] [diff] [review]
patch v1.1

Mento I'd like you to look at this also.
Attachment #210174 - Flags: superreview?(mark)

Comment 7

11 years ago
Comment on attachment 210174 [details] [diff] [review]
patch v1.1

>Index: nsGlueLinkingOSX.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsGlueLinkingOSX.cpp,v
>retrieving revision 1.1.2.1
>diff -u -6 -p -r1.1.2.1 nsGlueLinkingOSX.cpp
>--- nsGlueLinkingOSX.cpp	9 Jan 2006 19:03:09 -0000	1.1.2.1
>+++ nsGlueLinkingOSX.cpp	30 Jan 2006 21:46:05 -0000
>@@ -42,70 +42,84 @@
> #include <mach-o/dyld.h>
> #include <sys/param.h>
> #include <stdlib.h>
> #include <string.h>
> #include <stdio.h>
> 
>+static const mach_header* sXULLibImage;

Initialize?

> GetFrozenFunctionsFunc
> XPCOMGlueLoad(const char *xpcomFile)
> {
>+    const mach_header* lib = nsnull;
>+
>     if (xpcomFile[0] != '.' || xpcomFile[1] != '\0') {
>         char xpcomDir[PATH_MAX];
>         if (realpath(xpcomFile, xpcomDir)) {
>             char *lastSlash = strrchr(xpcomDir, '/');
>             if (lastSlash) {
>                 *lastSlash = '\0';
> 
>                 XPCOMGlueLoadDependentLibs(xpcomDir, ReadDependentCB);
>+
>+                sprintf(lastSlash, "/" XUL_DLL);

Buffer overflow waiting to happen?

>+                sXULLibImage = NSAddImage(xpcomDir,
>+                              NSADDIMAGE_OPTION_RETURN_ON_ERROR |
>+                              NSADDIMAGE_OPTION_WITH_SEARCHING |
>+                              NSADDIMAGE_OPTION_MATCH_FILENAME_BY_INSTALLNAME);

Return nsnull if !sXULLibImage - or explain if you had something else in mind.

Is XPCOMGlueLoad called more than once per process?

>             }
>         }
> 
>-        (void) NSAddImage(xpcomFile,
>-                          NSADDIMAGE_OPTION_RETURN_ON_ERROR |
>-                          NSADDIMAGE_OPTION_WITH_SEARCHING |
>-                          NSADDIMAGE_OPTION_MATCH_FILENAME_BY_INSTALLNAME);
>-        // We don't really care if this fails, as long as we can get
>-        // NS_GetFrozenFunctions below.
>+        lib = NSAddImage(xpcomFile,
>+                         NSADDIMAGE_OPTION_RETURN_ON_ERROR |
>+                         NSADDIMAGE_OPTION_WITH_SEARCHING |
>+                         NSADDIMAGE_OPTION_MATCH_FILENAME_BY_INSTALLNAME);

You can return nsnull here if !lib rather than trying to pass it to NSLookupSymbolInImage below.

>     }
> 
>-    if (!NSIsSymbolNameDefined("_NS_GetFrozenFunctions"))
>-      return nsnull;
>+    NSSymbol sym = NSLookupSymbolInImage(lib, "_NS_GetFrozenFunctions",
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_BIND |
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_RETURN_ON_ERROR);
>+    if (!sym)
>+        return nsnull;
> 
>-    NSSymbol sym = NSLookupAndBindSymbol("_NS_GetFrozenFunctions");
>     return (GetFrozenFunctionsFunc) NSAddressOfSymbol(sym);
> }
> 
> void
> XPCOMGlueUnload()
> {
>   // nothing to do, since we cannot unload dylibs on OS X
> }
> 
> nsresult
> XPCOMGlueLoadXULFunctions(const nsDynamicFunctionLoad *symbols)
> {
>+    // We don't null-check sXULLibHandle because this might work even
>+    // if it is null

Wait.  What?  Do you mean sXULLibImage?  Are you working with flat namespaces?

>     nsresult rv = NS_OK;
>     while (symbols->functionName) {
>         char buffer[512];
>         snprintf(buffer, sizeof(buffer), "_%s", symbols->functionName);
> 
>-        if (!NSIsSymbolNameDefined(buffer)) {
>-            rv = NS_ERROR_LOSS_OF_SIGNIFICANT_DATA;
>-        }
>-        else {
>-            NSSymbol sym = NSLookupAndBindSymbol(buffer);
>+        NSSymbol sym = NSLookupSymbolInImage(sXULLibImage, buffer,
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_BIND |
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_RETURN_ON_ERROR);
>+        if (sym)
>             *symbols->function = (NSFuncPtr) NSAddressOfSymbol(sym);
>-        }
>+        if (!*symbols->function)
>+            rv = NS_ERROR_LOSS_OF_SIGNIFICANT_DATA;

This seems weird to me.  Are you expecting *symbols->function to be set to something meaningful (like null) even before calling NSLookupSymbolInImage?
Attachment #210174 - Flags: superreview?(mark) → superreview-
(Assignee)

Comment 8

11 years ago
(In reply to comment #7)
> You can return nsnull here if !lib rather than trying to pass it to
> NSLookupSymbolInImage below.
> Wait.  What?  Do you mean sXULLibImage?  Are you working with flat namespaces?

Turns out I was confused about the documentation for |NSLookupSymbolInImage|.  I thought if the first argument was null, it would handle it like |dlsym| does (search global symbol table).  I'll fix this, and the rest of the issues, tomorrow.
Assignee: dougt → jhpedemonte
Status: ASSIGNED → NEW
> >+static const mach_header* sXULLibImage;
> 
> Initialize?

statics are auto-initialized to 0, and per brendan it's good form to not specify the initializer explicitly.

> Return nsnull if !sXULLibImage - or explain if you had something else in mind.

Yeah, we definitely *don't* want to fail if !sXULLibImage, but continue gracefully.

> Is XPCOMGlueLoad called more than once per process?

Well, not normally, but it's possible that multiple sharedlibs which statically link libpcomglue.a could call it independently (and would have separate statics).

> You can return nsnull here if !lib rather than trying to pass it to
> NSLookupSymbolInImage below.

We need to make sure that the "xpcomLib" argument passed can be null or "."; that is an old use-case of the glue which must be preserved. See http://lxr.mozilla.org/mozilla/source/xpcom/glue/standalone/nsGlueLinkingDlopen.cpp#108

In this case we should fallback to getting NS_GetFrozenFunctions using the flat namespace.

Comment 10

11 years ago
OK, in that case, you'll need to call NSLookupSymbolInImage if you have a non-null struct mach_header in hand and NSLookupAndBindSymbol otherwise.  NSLookupAndBindSymbolWithHint is another option, but I'd prefer to avoid it.  The search through the entire symbol table is expensive, so I'd prefer not doing it if possible when you know what library you want the symbol from.

Unfortunately, the dlopen family isn't an option because it doesn't work before 10.3.

NSLookupSymbolInImage only searches globally when a flat namespace is in use.  Flat isn't the default and should be avoided, it really only exists for compatibility with older code that depends on it.
(Assignee)

Comment 11

11 years ago
Created attachment 210247 [details] [diff] [review]
patch v1.2
Attachment #210174 - Attachment is obsolete: true
Attachment #210247 - Flags: review?(mark)
Attachment #210174 - Flags: review?(benjamin)

Comment 12

11 years ago
Comment on attachment 210247 [details] [diff] [review]
patch v1.2

>+
>+                snprintf(lastSlash, PATH_MAX - strlen(xpcomDir),"/" XUL_DLL);
----->                                               nit: space ^^

Would you mind fixing the dlopen and other implementations here too?

>+    NSSymbol sym;
>+    if (lib) {
>+        sym = NSLookupSymbolInImage(lib, "_NS_GetFrozenFunctions",
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_BIND |
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_RETURN_ON_ERROR);

Comparing this to the dlopen implementation, I think you should set sXULLibImage = nsnull here, or call XPCOMGlueUnload() and augment that to set the static for you.  This probably doesn't make a difference for well-behaved callers, but that might be my ignorance of libxpcom_glue speaking.

>+        NSSymbol sym;
>+        if (sXULLibImage) {
>+            sym = NSLookupSymbolInImage(sXULLibImage, buffer,
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_BIND |
>+                                 NSLOOKUPSYMBOLINIMAGE_OPTION_RETURN_ON_ERROR);
>+        } else {
>+            sym = NSLookupAndBindSymbol(buffer);
>         }

Since you do this twice now, I think this would be better as a static function, letting the compiler do the inlining.

r=me with those changes, over to Benjamin.
Attachment #210247 - Flags: review?(mark)
Attachment #210247 - Flags: review?(benjamin)
Attachment #210247 - Flags: review+
> Comparing this to the dlopen implementation, I think you should set
> sXULLibImage = nsnull here, or call XPCOMGlueUnload() and augment that to set

This isn't necessary (on mac) because .dylibs cannot be unloaded, so we don't have to worry about "state" changes in XPCOMGlueUnload.

Comment on attachment 210247 [details] [diff] [review]
patch v1.2

>-    if (!NSIsSymbolNameDefined("_NS_GetFrozenFunctions"))
>-      return nsnull;

You removed this hunk without adding it back below. You need it, because NSLookupAndBindSymbol traps if the symbol is not found.

>+    // the global symbol table.  If we couldn't get a mach_header pointer
>+    // for the XPCOM dylib, then use |NSLookupAndBindSymbol| to search the
>+    // global symbol table (this shouldn't normally happen).

This is not quite true. It is legal to call XPCOMGlueStartup(".") and rely on libxpcom.dylib already being loaded.

>+        if (sXULLibImage) {

You don't need to do flat-namespace lookup here... if we haven't found the XUL lib, you can just hard-fail (return NS_ERROR_FAILURE). If you want to keep flat-namespace lookup you need the same NSIsSymbolNameDefined code as above.
Attachment #210247 - Flags: review?(benjamin) → review-

Comment 15

11 years ago
> > Comparing this to the dlopen implementation, I think you should set
> > sXULLibImage = nsnull here, or call XPCOMGlueUnload() and augment that to set
> 
> This isn't necessary (on mac) because .dylibs cannot be unloaded, so we don't
> have to worry about "state" changes in XPCOMGlueUnload.

You're right, I was reading that snippet as plucking a symbol from sXULLibImage instead of lib.

>>-    if (!NSIsSymbolNameDefined("_NS_GetFrozenFunctions"))
>>-      return nsnull;
>
>You removed this hunk without adding it back below. You need it, because
>NSLookupAndBindSymbol traps if the symbol is not found.

I know that's the documented behavior, but when I tested it, I found that NSLookupAndBindSymbol doesn't trap on failure as advertised, but instead returns null.  This is on 10.4.4 ppc and x86, your mileage may vary.
I can guarantee that it trapped when I originally wrote the code on 10.3.something.
(Assignee)

Comment 17

11 years ago
Created attachment 210260 [details] [diff] [review]
patch v1.3

|NSLookupAndBindSymbol| also doesn't trap for me (10.4.4PPC), but better safe than sorry (for older OSes).  Actually, I don't like also using |NSIsSymbolNameDefined|, because that means it looks through the global symbol table twice.  However, based on an internet search, it looks like |NSInstallLinkEditErrorHandlers| is broken in 10.4.
Attachment #210247 - Attachment is obsolete: true
Attachment #210260 - Flags: review?(benjamin)
Comment on attachment 210260 [details] [diff] [review]
patch v1.3

Be careful with nsGlueLinkingWin, you will probably need to #define snprintf _snprintf (or just use _snprintf directly).
Attachment #210260 - Flags: review?(benjamin) → review+
(Assignee)

Updated

11 years ago
Attachment #210260 - Flags: superreview?(mark)

Comment 19

11 years ago
Comment on attachment 210260 [details] [diff] [review]
patch v1.3

r=me with _snprintf
Attachment #210260 - Flags: superreview?(mark) → superreview+
(Assignee)

Comment 20

11 years ago
Checked in to trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

11 years ago
Comment on attachment 210260 [details] [diff] [review]
patch v1.3

Trunk tinderboxen are all green, so asking for approval on the branches.
Attachment #210260 - Flags: branch-1.8.1?(benjamin)
Attachment #210260 - Flags: approval1.8.0.2?
Comment on attachment 210260 [details] [diff] [review]
patch v1.3

Recommending approval1.8.0.2 based on the fact that this is standalone-glue-only and therefore won't affect ffox/tbird, seamonkey doesn't use the standalone glue on mac, and this is verified on trunk, and fixes a JavaXPCOM embedding bug on mac.
Attachment #210260 - Flags: branch-1.8.1?(benjamin) → branch-1.8.1+
Comment on attachment 210260 [details] [diff] [review]
patch v1.3

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210260 - Flags: approval1.8.0.2? → approval1.8.0.2-
Comment on attachment 210260 [details] [diff] [review]
patch v1.3

Correcting dveditz typo.
Attachment #210260 - Flags: approval1.8.0.2- → approval1.8.0.2+
(Assignee)

Comment 25

11 years ago
Checked in to both branches.
Keywords: fixed1.8.0.2, fixed1.8.1
marking [nvn-dl], which removes this bug from the "to be verified by QA" list
for Firefox 1.5.0.2.
Whiteboard: [nvn-dl]
You need to log in before you can comment on or make changes to this bug.