121521 - ABR doing 'File->Open...' to open a file from disk

Status: VERIFIED FIXED

(Core :: Networking: File, defect, P2)

Description

[John Morrison]

ABR doing 'File->Open...' to open a file from disk

Steps to Reproduce:
1) Start Mozilla and do 'File->Open...'
2) When the native win32 filepicker comes up, select a disk file
   (text or html; doesn't matter) and click on 'Open' button

Actual Results: 
    [E] ABR: Array bounds read in memchr {1 occurrences}

See attached Purify details

Comment 1

[John Morrison]

Attachment: [E] ABR: Array bounds read in memchr {2 occurrences}

Comment 2

[Darin Fisher]

this is a crash waiting to happen... jrgm: thx for locating this one.

Comment 3

[Andreas Otte]

Enlighten me please, first time I see a purify log. I don't get it, why do we
reach the array bounds in memchr?
 

Comment 4

[Darin Fisher]

it must be the case that we are dereferencing memory that we do not own.  one of
the parameters to memchr must be invalid.

Comment 5

[John Morrison]

I put |printf("specLen: %d,\tspec: '%s'\n", specLen, spec);| in just before
the memchr. When I hit the "Open" button, all I see is (for 'H:\foo.txt'):

  specLen: 12,    spec: '//H:/foo.txt'
  specLen: 2,     spec: '//'

Comment 6

[John Morrison]

Yawn. This happens when spec is '//' and specLen is 2. Essentially, Purify 
is whining about this

    const char *p = (const char *) memchr(ptr, '/', 0);

if ptr is '.get()' from a nsCAutoString. i.e., it's confused by the string-fu, 

But it doesn't seem to be a real error. 'memchr' returns NULL anyways and the 
use of p is nullchecked in the rest of the scope. 

So, unless one of you has a concern about why we are parsing '//' for 
additional parts, then you can just mark this INVALID, I suppose.

Comment 7

[Darin Fisher]

Attachment: v1.0 patch

yeah, let's not risk passing a junk pointer into memchr.

Comment 8

[Darin Fisher]

andreas: can you r= this patch?

Comment 9

[Scott MacGregor]

Comment on attachment 67031 [details] [diff] [review]
v1.0 patch

sr=mscott

Comment 10

[John Morrison]

Comment on attachment 67031 [details] [diff] [review]
v1.0 patch

r=jrgm, if you want it :-)

Comment 11

[Andreas Otte]

looks good to me too, r=[e-mail]

Comment 12

[Darin Fisher]

fixed-on-trunk

Comment 13

[benc]

darin:
can you mark verified as a "code fix"?
TIA

Comment 14

[Darin Fisher]

v.

Comment 15

[Darin Fisher]

v. - one more time