241296 - Add "/usr/lib" for "libatk-bridge.so" searching

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

In LoadGtkModule, mozilla use PR_GetLibraryPath to get library searching path.
But  the return value of PR_GetLibraryPath is actually value of
"LD_LIBRARY_PATH". If "LD_LIBRARY_PATH" is not set, mozilla can't find
"gtk-2.0/modules/libatk-bridge.so" under default library path "/usr/lib".

Comment 1

[Louie Zhao]

Attachment: patch v1

Comment 2

[tenthumbs]

Altering the system search rules is always risky. Altering the rules with a
system library is particularly so. You never know if some other directory
contains the preferred version of some library. If you do this (and you
shouldn't, any failure is a system issue) then you need to evaluate the
security implications very carefully.

Comment 3

[Kyle Yuan]

ccing Bill for some comments.

Comment 4

[Bill Haneman]

I agree that something doesn't make sense here; there's no guarantee that
/usr/lib will contain the correct bridge either.  For instance, on my system, if
you searched /usr/lib you'd get the wrong version of the bridge, and things
would break.

Isn't it OK to just use LD_LIBRARY_PATH, and use the (existing) system search
rules if there's no LD_LIBRARY_PATH?

Comment 5

[Louie Zhao]

Attachment: patch v2

Thanks for pointing this out. In this patch, I append "/usr/lib" to the path.
Then "/usr/lib" will be the last one to try if all the precedents failed.

> Altering the system search rules is always risky.

This is true. But the code here won't interfere with system search rules.
In order to enable Accessiblity feature, Mozilla need "libatk-bridge.so", which
locates under [system library path]/gtk-2.0/modules. Since this path normally
doesn't appear in library searching path, mozilla gets library searching path
first, then append "gtk-2.0/modules" to each item to try to load such library.
So the code only affect the rule of "libatk-bridge.so" searching.

> you need to evaluate the security implications very carefully.
Actually the change here is equal to "export
LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib" before running mozilla. So it won't
add any security holes.

> there's no guarantee that /usr/lib will contain the correct bridge either.
This is quite true. The patch just add an opportunity to try, which could be
the right one in most cases(but has been missed by default).

> Isn't it OK to just use LD_LIBRARY_PATH, and use the (existing) system search

rules if there's no LD_LIBRARY_PATH?
Since LD_LIBRARY_PATH won't be set for normal user (we expect mozilla can run
with "accessibled-enabled" without any additional configuration), the value
PR_GetLibraryPath returns will be empty. That's why the patch add "/usr/lib",
which is the default library search path.

There is a more accurate solution, but it will depend on "pkg-config":
use "pkg-config --variable libdir gtk+-2.0" to get library path of gtk package.


Any comment on this one ?

Comment 6

[Louie Zhao]

Bill, using appending "/usr/lib" instead of inserting "/usr/lib" will solve the
problem you mentioned in #4. Do you have any comment for this solution (please
see "patch v2")?

Comment 7

[Bill Haneman]

using "appended /usr/lib" seems OK to me.

Comment 8

[timeless]

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

>--- accessible/src/atk/nsAppRootAccessible.cpp	25 Mar 2004 18:36:08 -0000	1.9
>@@ -771,7 +771,8 @@
>         char *curLibPath = PR_GetLibraryPath();
...
>         PR_FreeLibraryName(curLibPath);

This is *not* a matched pair of alloc/free.

That free function is for:
519 PR_GetLibraryName(const char *path, const char *lib)
see comment:

566 ** Free the memory allocated, for the caller, by PR_GetLibraryName
567 */
568 PR_IMPLEMENT(void) 
569 PR_FreeLibraryName(char *mem)

--
426 PR_GetLibraryPath(void)
uses strdup, the pair for which is |free| (or something which will call the
|free| that lives in nspr...

why not fiddle with things the way everyone else does:

http://lxr.mozilla.org/seamonkey/source/build/unix/run-mozilla.sh#329

Comment 9

[Louie Zhao]

> 426 PR_GetLibraryPath(void)
> uses strdup, the pair for which is |free| (or something which will call the
> |free| that lives in nspr...
> 
Since there is no corresponding free function for PR_GetLibraryPath, using
PR_FreeLibraryName has no harm because it's actually to invoke "free()". Please
refer to http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/malloc/prmem.c#459

> why not fiddle with things the way everyone else does:
> 
> http://lxr.mozilla.org/seamonkey/source/build/unix/run-mozilla.sh#329
> 
Since only Accessibility module has this issue, fixing it here will be better
than spreading it to whole mozilla.

Comment 10

[timeless]

you conveniently missed the branch where PR_Free uses its own (de)allocator for
the release. the code is wrong, violates various contracts and should be fixed.

Comment 11

[Louie Zhao]

Timeless, I have filed bug243995 to deal with "PR_FreeLibraryName" since it's
not part of the issue we discussed in this bug.

Comment 12

[Henry Jia]

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

sr=Henry

Pls handle bug 243995 ASAP.

Comment 13

[Louie Zhao]

thanks for sr and r. Patch checked in trunk.