Purify IPR in nsGlobalWindow::WinHasOption(...)




DOM: Core & HTML
18 years ago
15 years ago


(Reporter: rpotts (gone), Assigned: rpotts (gone))


({dom0, helpwanted})

dom0, helpwanted

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [rtm-][HAVE FIX][can't repro])


(1 attachment)



18 years ago
hey brendan,

I'm seeing a whole bunch of purify invalid pointer reads during mozilla startup.

I tracked it down a little and it looks like the following is happening:
- GlobalWindowImpl::OpenInternal(...) sets options = JSGetStringBytes(str)
- Everything is fine until LoadURI is called.  see 
- after the call to LoadURI, the JS string cache seems to have lost 'options' so  
its subsequent usage in SizeOpenedDocShellItem(...) causes invalid memory 

The call to LoadURI is rather new (9/20) so I assume that until then, everything 
was working fine...

It looks like 'options' needs to be strdup'ed to ensure its lifetime...

-- rick

Purify Error: (one of many)
[E] IPR: Invalid pointer read in GlobalWindowImpl::WinHasOption(char *,char 
const*,int,int *) {1 occurrence}
    Reading 1 byte from 0x0719c150 (1 byte at 0x0719c150 illegal)
    Address 0x0719c150 points into a HeapAlloc'd block in unallocated region of 
heap 0x03180000
    Thread ID: 0xe0
    Error location
        GlobalWindowImpl::WinHasOption(char *,char const*,int,int *) 
                  PRInt32 found = 0;
                  while (PR_TRUE) {
             =>     while (nsCRT::IsAsciiSpace(*aOptions))
                    comma = strchr(aOptions, ',');
        GlobalWindowImpl::SizeOpenedDocShellItem(nsIDocShellTreeItem *,char 
*,UINT) [nsGlobalWindow.cpp:3309]
                  PRBool positionSpecified = PR_FALSE;
                  PRInt32 temp;
             =>   if ((temp = WinHasOption(aFeatures, "left", 0, &present)) || 
                    chromeX = temp;
                  else if ((temp = WinHasOption(aFeatures, "screenX", 0, 
&present)) || present)
                    chromeX = temp;
        GlobalWindowImpl::OpenInternal(JSContext *,long 
*,UINT,int,nsIDOMWindowInternal * *) [nsGlobalWindow.cpp:3093]
                  if (windowIsNew)
             =>     SizeOpenedDocShellItem(newDocShellItem, options, 
                  if (windowIsModal) {
                    nsCOMPtr<nsIDocShellTreeOwner> newTreeOwner;
        GlobalWindowImpl::OpenDialog(JSContext *,long 
*,UINT,nsIDOMWindowInternal * *) [nsGlobalWindow.cpp:2039]
        OpenWindow     [nsAppRunner.cpp:248]
        LaunchApplication [nsAppRunner.cpp:359]
        startupPrefEnumerationFunction [nsAppRunner.cpp:486]
        nsPref::EnumerateChildren(char const*,(*)(char const*,void *),void *) 
        HandleArbitraryStartup [nsAppRunner.cpp:538]
        DoCommandLines [nsAppRunner.cpp:586]
It sounds like the GC ran and the string was GC'd, which will remove any bytes
deflated from its unicode chars.  So we coul fix with strdup'ing, which would
also help because WinHasOption assumes non-const-ness and temporarily stores
NULs to terminate substrings.

I'm gonna weasel out of this one (unless jst weasels harder) -- too much mozilla
work along with bugs like bug 53123 for me right now.



Assignee: brendan → jst

Comment 2

18 years ago
Adding some random keywords...

Since this is multiple free memory reads, I really think that we should fix it 
for 6.0
Keywords: rtm

Comment 3

18 years ago
Rick, what's the user impact of this?
Whiteboard: [need info]

Comment 4

18 years ago
The 'user impact' of this is a potential crash.  

We are accessing memory that has been *freed* - This is bad (that's why purify 
puts little red stop signs next to them).  To make things worse, we are 
accessing the memory many times!

The only thing worse than a Invalid Pointer Read (IPR) is an Invalid Pointer 

we can crash either when the pointer is dereferenced or when we pass this bad 
memory off to libc routines and pretend that it is really a string...

The fix is trivial...  Please at least fix it in the trunk - or reassign it to 
me so I can fix it on the trunk.

-- rick

Comment 5

18 years ago
Marking rtm+ need info.  Johnny has a fix for this.  Please get it reviewed and
super-reviewed. Thanks!
Whiteboard: [need info] → [rtm+ need info]

Comment 6

18 years ago
Marking this as a P2...
Priority: P3 → P2


18 years ago
Whiteboard: [rtm+ need info] → [rtm+ need info][HAVE FIX]

Comment 8

18 years ago
Heikki, two more things need to happen with this bug.

1) Please verify with purify (this rhymes!  :-))
2) Get a super-review from vidur or brendan or waterson.

Re-assigning to you as Johnny (as usual) has too many bugs on his plate!  :)
Assignee: jst → heikki
Building purifyable executable...
Hmm, Rick, I never seem to hit this in Purify... maybe I never execute this code
(haven't checked yet). What is your start page? Sidebar open/closed? What is in
sidebar? Which skin (I am using modern)? Did you have some unusual Purify settings?

Added Bruce to Cc. Bruce, do you see this?
Well, placing breakpoints in debugger I do seem to hit this code, but Purify 
does not find anything wrong...
I can't reproduce what Rick Potts is seeing. The patch seems correct, but I
would not want to check it in if I had no means to verify that it actually fixed
something. Rick, or someone, if you see it please feel free to either take this
or tell me what I need to do to see this.
Keywords: helpwanted
Whiteboard: [rtm+ need info][HAVE FIX] → [rtm+ need info][HAVE FIX][can't repro]

Comment 13

18 years ago
Rick, since Heikki cannot reproduce this, could you apply the patch in your tree
and update the bug with your findings and be the reviewer of the patch.
Assignee: heikki → rpotts
Whiteboard: [rtm+ need info][HAVE FIX][can't repro] → [rtm need info][HAVE FIX][can't repro]

Comment 14

18 years ago
No comments for 23 days. Can we please get this off the rtm radar?

Comment 15

18 years ago
rpotts: Can we take this off of the rtm radar?

Comment 16

18 years ago
sure...  I don'r have time to look at it :-(  Since no one else can reproduce it 
, lets forget about it for now...

-- rick
Keywords: rtm

Comment 17

18 years ago
Moving to rtm- per rpotts.
Whiteboard: [rtm need info][HAVE FIX][can't repro] → [rtm-][HAVE FIX][can't repro]
Keywords: dom0


17 years ago
Target Milestone: --- → Future

Comment 18

15 years ago
This code in no way resembles what it used to look like, and Purify doesn't see
the error.

I don't see a reason to keep the bug around.
Last Resolved: 15 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.