The default bug view has changed. See this FAQ.

libpng conflicts with system libpng in static builds

VERIFIED FIXED

Status

()

Core
ImageLib
VERIFIED FIXED
15 years ago
11 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Glenn Randers-Pehrson)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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.
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).

(Reporter)

Comment 2

15 years ago
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....
> 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.
(Reporter)

Comment 4

15 years ago
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?
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

14 years ago
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 ?
(Assignee)

Comment 7

14 years ago
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.
(Assignee)

Comment 8

14 years ago
> 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

14 years ago
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.
(Assignee)

Updated

14 years ago
Blocks: 119934
(Assignee)

Comment 10

14 years ago
Reassigning to myself and setting as blocker of bug#119934.  It's almost a dupe
but I'd prefer to work on libpng separately.
Assignee: bryner → randeg
(Assignee)

Comment 11

14 years ago
accepting bug.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

14 years ago
Created attachment 128779 [details] [diff] [review]
Patch to prefix all libpng exported symbols with "moz_"
(Assignee)

Updated

14 years ago
Blocks: 208607
(Assignee)

Comment 13

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

r?
Attachment #128779 - Flags: review?(pavlov)

Updated

14 years ago
Attachment #128779 - Flags: superreview?(tor)
Attachment #128779 - Flags: review?(pavlov)
Attachment #128779 - Flags: review+

Updated

14 years ago
Attachment #128779 - Flags: superreview?(tor) → superreview+
(Assignee)

Comment 14

14 years ago
Created attachment 134319 [details] [diff] [review]
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.
Attachment #128779 - Attachment is obsolete: true
(Assignee)

Comment 15

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

Does this need r/sr again for this trivial change?
(Reporter)

Comment 16

14 years ago
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.
Attachment #134319 - Flags: superreview?
(Reporter)

Comment 17

14 years ago
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.
Attachment #134319 - Flags: superreview+
(Reporter)

Comment 18

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

oops, i confused bugzilla
Attachment #134319 - Flags: superreview?
(Reporter)

Comment 19

14 years ago
Checked in.  Thanks for the patch Glenn.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.