181936 - libpng conflicts with system libpng in static builds

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Brian Ryner (not reading)]

It's fairly easy to get a libpng runtime version conflict if you use a static
build of Mozilla or Phoenix with a gtk theme engine (i.e. the pixmap engine)
that dlopen()s the system libpng library.  The main problem is that calls from
inside the system libpng use the normal symbol scope lookup and end up finding
_some_ of the symbols in the mozilla binary.  The symptom is that png's fail to
load for the gtk theme engine, which will cause pixmap backgrounds not to show
up for nsITheme-drawn elements.

It should be possible to fix this in libpng by not using exported symbols
internally, but since we can't really change the system libpng's that are out
there, we need to come up with another solution.

Hiding the libpng symbols in the final binary sounds appealing, but the linker
doesn't support doing that.  A solution that would work is to rename (via
#defines, likely) the libpng symbols that Mozilla uses.  That prevents any
possibility of the system libpng using those symbols by mistake.

This may affect libjpeg as well, but I haven't tested it yet.

Comment 1

[hacker formerly known as [e-mail]]

Sounds sickingly familiar to bug 119934 though I'd wonder why you don't just use
the system png library in this case (assuming that it's a compatible version).

Comment 2

[Brian Ryner (not reading)]

That's the problem.  We don't know if it will be compatible on the system the
build runs on.

I wonder if it's time to revisit system libpng as an external requirement....

Comment 3

[hacker formerly known as [e-mail]]

> The main problem is that calls from
> inside the system libpng use the normal symbol scope lookup and end up finding
> _some_ of the symbols in the mozilla binary.

Hrm.  Have you tried building w/o using -Bsymbolic?  Or is the problem that the
system version of libpng should be built using -Bsymbolic and isn't?  If we made
the in-tree version of libpng dynamic instead of static, would that "fix" the
problem or just cause new problems as the theme engine would use the symbols
entirely from the moz version of libpng instead of the system version?

Having libpng be an external dependency seems like the cleanest solution.

Comment 4

[Brian Ryner (not reading)]

If the system libpng was built with -Bsymbolic, that would fix the problem. 
However, there's not much we can do about that.

We could build Mozilla's png decoder as a dynamic component with libpng
statically linked; that would keep the symbols from being visible to the system
libpng.  But I don't really want to make even more things forced-dynamic in a
static build.

Are versions of libpng more recent than the one we're using fully backwards
compatible?  Are there any custom fixes that we need?

Comment 5

[hacker formerly known as [e-mail]]

http://lxr.mozilla.org/mozilla/source/modules/libimg/png/MOZCHANGES

Recent versions are compatible for the moment as tor recently landed version
1.2.5.  But seeing as we were back at 1.0.x for 3 yrs or so, there's a good
chance that we'll be incompatible in the future.

Comment 6

[Roland Mainz]

What about modifying the libpng.h/libjpeg.h header file(s) and do a |#define
foo(a,b) moz_foo((a), (b))| to work around the symbol issue ?

Comment 7

[Glenn Randers-Pehrson]

The libpng distributed with mozilla is incompatible with the system libpng-1.2.5
because it is no longer pristine.  Unused functions and some unused members of
the png_ptr and png_info structs were removed in bug #208607.  The library still
calls itself version 1.2.5, though.

Comment 8

[Glenn Randers-Pehrson]

> What about modifying the libpng.h/libjpeg.h header file(s) and do a |#define
> foo(a,b) moz_foo((a), (b))| to work around the symbol issue ?

Why not simply #define foo moz_foo ?  I tried this in a private copy of libpng
and it seems to work.

Comment 9

[Roland Mainz]

Glenn Randers-Pehrson wrote:
> > What about modifying the libpng.h/libjpeg.h header file(s) and do a |#define
> > foo(a,b) moz_foo((a), (b))| to work around the symbol issue ?
>
> Why not simply #define foo moz_foo ?  I tried this in a private copy of libpng
> and it seems to work.

That would work, too - however I prefer the first form since it allows to catch
syntax errors (for example "wrong number of function arguments" etc.) without
pain.

Comment 10

[Glenn Randers-Pehrson]

Reassigning to myself and setting as blocker of bug#119934.  It's almost a dupe
but I'd prefer to work on libpng separately.

Comment 11

[Glenn Randers-Pehrson]

accepting bug.

Comment 12

[Glenn Randers-Pehrson]

Attachment: Patch to prefix all libpng exported symbols with "moz_"

Comment 13

[Glenn Randers-Pehrson]

Comment on attachment 128779 [details] [diff] [review]
Patch to prefix all libpng exported symbols with "moz_"

r?

Comment 14

[Glenn Randers-Pehrson]

Attachment: Patch to prefix all libpng externals with MOZ_PNG

Changed the prefix from "moz_png" to "MOZ_PNG" to be consistent with
bug #119934 which uses "MOZ_EXP" for expat externals.

Comment 15

[Glenn Randers-Pehrson]

Oops, expat uses "MOZ_XML" not "MOZ_EXP" but the argument still applies.

Does this need r/sr again for this trivial change?

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 134319 [details] [diff] [review]
Patch to prefix all libpng externals with MOZ_PNG

sr=bryner for the trivial change, and I wouldn't bother with a second review.

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 134319 [details] [diff] [review]
Patch to prefix all libpng externals with MOZ_PNG

sr=bryner for the trivial change, and I wouldn't bother with a second review.

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 134319 [details] [diff] [review]
Patch to prefix all libpng externals with MOZ_PNG

oops, i confused bugzilla

Comment 19

[Brian Ryner (not reading)]

Checked in.  Thanks for the patch Glenn.