If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

ILTRACE core dump in gif.cpp due to LOCAL il_log_module var

VERIFIED FIXED in mozilla0.9

Status

()

Core
ImageLib
P1
critical
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: harbaugh, Assigned: cls)

Tracking

Trunk
mozilla0.9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
(NOTE: proposed fix is included)
'il_log_module' is re-declared locally in gif.cpp and set to NULL.
This causes an attempt to dereference a NULL pointer in the ILTRACE
expansion (files gif.cpp, jpeg.cpp and ipng.cpp).  Hence the program
segfaults before displaying a window.


PROPOSED FIX:
  remove lines 80-81 of gif.cpp, which are actually a *LOCAL*
  redeclaration of variables that should be GLOBAL

    80:   int il_debug;
    81:   PRLogModuleInfo *il_log_module = NULL;

  The 'true' declaration/initialization of these variables occurs
  in ilclient.cpp.  Similar lines should be removed from jpeg.cpp
  and ipng.cpp

  line 80 factors into this because the local 'il_debug' is
  not initialized and hence can contain random junk

========= analysis ===========
Debugger shows that SEGV occurs upon reaching line 859 of gif.cpp:

% ./mozilla -g -d ladebug 
     ./run-mozilla.sh -g -d ladebug ./mozilla-bin 
     MOZILLA_FIVE_HOME=.
      
LD_LIBRARY_PATH=.:/usr/local/fbscapp/inst/gtk128/lib:/usr/local/fbscapp/inst/ORBit051/lib:/usr/local/fbscapp/inst/glib128/lib:/usr/shlib:/usr/ccs/lib:/usr/lib/cmplrs/cc:/
usr/lib:/var/shlib
          LIBRARY_PATH=.
            SHLIB_PATH=.
               LIBPATH=.
            ADDON_PATH=.
           MOZ_PROGRAM=./mozilla-bin
           MOZ_TOOLKIT=
             moz_debug=1
          moz_debugger=ladebug
     /usr/bin/ladebug ./mozilla-bin 
     Welcome to the Ladebug Debugger Version 4.0-62 (built Jun 23 2000)
     ------------------ 
     object file name: ./mozilla-bin 
     core file name: 
     Warning: cannot open core file : 
     Reading symbolic information ...done
     (ladebug) use ../../modules/libimg/gifcom
     Directory search path for source files:
      . ../../modules/libimg/gifcom
     (ladebug) run
     nsNativeComponentLoader: autoregistering begins.
     nsNativeComponentLoader: autoregistering succeeded
     nNCL: registering deferred (0)
     GFX: dpi=96 t2p=0.0666667 p2t=15 depth=16
     WEBSHELL+ = 1
     Initialized app shell component {18c2f989-b09f-11d2-bcde-00805f0e1353},
rv=0x00000000
     Initialized app shell component {33e569b0-40f8-11d4-9a41-000064657374},
rv=0x00000000
     ProfileName : default
     ProfileDir  : /users/primgr/harbaugh/.mozilla/default
     CSSLoaderImpl::LoadAgentSheet: Load of URL
'file:///users/primgr/harbaugh/.mozilla/default/chrome/user.css' failed.  Error
code: 16389
     WEBSHELL+ = 2
     Enabling Quirk StyleSheet
     Note: verifyreflow is disabled
     Note: styleverifytree is disabled
     Note: frameverifytree is disabled
     Enabling Quirk StyleSheet
     WARNING: chrome: failed to get base url for
chrome://necko/locale/necko.properties -- using wacky default, file
nsChromeRegistry.cpp, line 507
     Obtained name of Personal Toolbar from bookmarks string bundle.
     Start reading in bookmarks.html
     Finished reading in bookmarks.html  (78125 microseconds)
     WEBSHELL+ = 3
     Thread received signal SEGV
     stopped at [int il_gif_write(il_container*, const PRUint8*, int32):859
0x3ffbf9140f8]
         859         ILTRACE(9,("il:gif: state %d len %d buf %u p %u q %u ep
%u",


     Preprocessing gif.cpp and examining the output shows *TWO* 
     declarations of il_log_module.  The first is an 'extern'
     from if_struct.h; the second is local (line 81 of gif.cpp)
     and sets il_log_module to NULL.

     Line 859 expands to

        { if(il_debug>1) {do { if (((il_log_module)->level >= (1))) {
PR_LogPrint ("il:gif: ...

     where the attempt to dereference (il_log_module)->level
     SEGFAULTS since the local il_log_module overrides the global.
(Reporter)

Updated

17 years ago
Severity: normal → critical
(Reporter)

Comment 1

17 years ago
I did test the proposed fix and it works.  The only
question is whether it will adversely affect other platforms.

Comment 2

17 years ago
Toni,
    I am attaching the preprocessed output of gif.cpp that you asked me for.
This is with the 6.1 compiler under Tru64 4.0D.

Comment 3

17 years ago
Created attachment 13801 [details]
gif.cpp preprocessor output

Comment 4

17 years ago
Created attachment 15122 [details] [diff] [review]
Patch to fix bug

Comment 5

17 years ago
pnunn: You look like the person who gets to approve this change if you like it,
and Blame says its your code anyway so thats even better.  I asked for a
reviewer on the newsgroup, so if they say it looks good and you say it looks
good I will check it in.
This is a slam dunk.  I can review and approve, be good if pnunn gave r= too. 
Please get this in before any branch is cut from the trunk.

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

17 years ago
I would like to see the lines removed as originally proposed instead of
littering libimg with "#if 0" junk.  Clean sweep alternative attached.

Comment 8

17 years ago
Created attachment 15169 [details] [diff] [review]
remove extra il_debug and il_log_module definitions

Comment 9

17 years ago
Brendan, can you review/approve tor's patch.  He found the same problem in
several other files.
(Assignee)

Comment 10

17 years ago
We also need to have these global symbol conflicts resolved for static
components (bug 46775) to work.
Blocks: 46775

Comment 11

17 years ago
r: pnunn
If someone else can check this in. I am swamped.
And some of you Mozillittes can check in easier than
I, a dreaded Netscapian, can.
Status: NEW → ASSIGNED
Target Milestone: --- → M18
a=brendan@mozilla.org, no brainer.

/be

Comment 13

17 years ago
Its already in my tree.  Ill check it (tor's patch) in as soon as the tree opens.

Comment 14

17 years ago
much thanks.
-p

Comment 15

17 years ago
Its checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 16

17 years ago
This patch breaks the mac.  Should I just #ifdef the code so that its only there
for that platform?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

17 years ago
Attaching new fix

Comment 18

17 years ago
Created attachment 15309 [details] [diff] [review]
A new fix, #ifdefed for mac

Comment 19

17 years ago
breandan || pnunn,
   Could one of you review this new patch?

Comment 20

17 years ago
Hmmm.  We're trying to make these local versions of the symbols go away, right?  
If they can't go away on Mac, then we've done something wrong somewhere.  This 
patch may be good enough to get the work in for the other platforms, but to me it 
looks like it means there's still more work to be done on Mac with respect to 
this bug.  I can only r= this patch on the condition that we're still working on 
the right completely cross-platform fix.

Comment 21

17 years ago
Updating QA Contact
QA Contact: paw → tpreston

Comment 22

17 years ago
Looks like these changes were checked in already ... and they break the Win32 
debug builds. With undefined symbols for il_log_module and il_debug.

You have to also undefine the ILTRACE() macro to get things going again.

Comment 23

17 years ago
As sheriff, I have backed out the changes, caused build bustage on win32, also I
think this can be done xp

Comment 24

17 years ago
    The phone company dug through DSL line the other day, so I have been
off the internet for a few days.  I was able to read my email this 
morning only to discover that I broke the debug windows builds on the
27th while trying to fix bug 50872.  I watched tinderbox after I did
that checkin, and they all stayed green so I was quite supprised this
morning to learn that I had broken things.  My guess is that both the
windows tinderbox machines are running optimized builds?  In any event,
after the tinderboxes went green I left, and was not available to help
sort out the bustage I had caused.  I am truly sorry for causing problems.
    This was my second attempt at fixing 50872, and given that I have
broken some platform each time I am now quite scared of trying again.
I am only able to test my code under Linux, and that is obviously not
sufficient for this code.  I am going to see if I can find someone at
NS who has access to all 3 platforms to take ownership of this bug.

Updated

17 years ago
Target Milestone: M18 → Future

Comment 25

17 years ago
Why not just remove those il_debug, il_log_module and ILTRACE from gif.cpp, 
ipng.cpp, jpeg.cpp. We just lost some debug information. That should solve
all the problem. If somebody believe it is really necessary to have those
information, we may have to define local version in different names, like
ipng_log_module, ipng_debug, IPNG_TRACE. For those small modules, I doubt
its necessity.

Updated

17 years ago
Blocks: 61525

Comment 26

17 years ago
*** Bug 62024 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Depends on: 70938

Comment 27

17 years ago
*** Bug 67865 has been marked as a duplicate of this bug. ***

Comment 28

17 years ago
Whats the progress on this bug were still seeing people having this crash on them.

Comment 29

17 years ago
There has been no progress as far as I know.  We know whats going on, and
several proposals to fix this have been put on the table.  Someone with some
athority (pnunn?) needs to pick one. FWIW, I like Shanjian Li's proposal the
best.  Lets just remove this code.  We dont need it and its causing problems.
(Assignee)

Comment 30

17 years ago
Created attachment 28027 [details] [diff] [review]
Create uniquely named debug variables for each img component
(Assignee)

Comment 31

17 years ago
Taking bug.
Assignee: pnunn → cls
Status: REOPENED → NEW
Keywords: mozilla0.9
OS: OSF/1 → All
Priority: P3 → P1
Hardware: DEC → All
Target Milestone: Future → mozilla0.9

Comment 32

17 years ago
Verified 03/18/01 ptach on Mac debug for gif, jpg, mng and png decoder projects 
as wells as libimg project.  Still need verification on opt build
(Assignee)

Comment 33

17 years ago
Verified debug & opt on win32.

Comment 34

17 years ago
sr=tor

Comment 35

17 years ago
r=pavlov
(Assignee)

Comment 36

17 years ago
Patch checked in.  Marking fixed.
Status: NEW → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 37

17 years ago
Verified fix checked into lxr.mozilla.org
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.