86885 - failure to load cookie service causes crash in nsHTMLDocument::GetCookie()

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Ken]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    

I am embedding gecko under Windows. In nsHTMLDocument.cpp, 
nsHTMLDocument::GetCookie() makes the assumption that the cookie service works. 
Unfortunately, the code added for revision 3.322 by [e-mail] will 
cause a crash if it doesn't.

Basically, a char* was not initialized to nsnull, but my proposed patch cleans 
things up a little more.


Reproducible: Always
Steps to Reproduce:
1. If you have the cookie service, remove it from the components
2. Load any website that uses cookies
3. Crash will occur on nsCRT::free(cookie)

Expected Results:  Do not assume cookie service can load. First truncate the 
referenced aCookie, then attempt to retrieve the new one and assign/free only 
if successful.

Comment 1

[Ken]

Attachment: patch to revision 3.348

Comment 2

[chris hofmann]

steve, can you have a look?  we need this for embedding.  thanks

Comment 3

[Chris Waterson]

These changes look fine to me (they are safer than the previous code, but 
appear to be equivalent in all other respects).

Looking at the code, I suspect that we've turned over a rock where a bunch of 
I18n issues are crawling. Can cookies contain non-ASCII characters?

Also, it sort of sucks that we have to copy the string twice to return it. scc, 
any suggestions for the string-fu?

Comment 4

[Stephen P. Morse]

No, cookies cannot contain non-ascii characters.  There is no i18n problem here.

Comment 5

[Stephen P. Morse]

r=morse on the patch

Comment 6

[Stephen P. Morse]

Waterson, since you already looked this over, would you like to super-review it?

Comment 7

[Chris Waterson]

I'll let scc do the honors, as he may have some ideas for improving the string 
usage.

Comment 8

[Johnny Stenback (:jst)]

+      str.AssignWithConversion(cookie);
+      nsCRT::free(cookie);
+      aCookie.Assign(str);

Should be:

CopyASCIItoUCS2(nsDependentCString(cookie), aCookie);
nsMemory::free(cookie);

to eliminate one string copy.

Comment 9

[Chris Waterson]

> No, cookies cannot contain non-ascii characters.  There is no i18n
> problem here.

Well, with the APIs that we have, people can certainly _set_ cookies with 
non-ASCII characters:

 nsHTMLDocument::SetCookie(const nsAReadableString& aCookie)

So, is the right behavior here to just completely corrupt the data (at line 
nsHTMLDocument.cpp:1992)? (Oy, is the right behavior to allocate a new 
PRUnichar buffer with the nsString() ctor, copy the data into the buffer, and 
then corrupt it?)

Gratuitous copies aside, I'd have thought UTF-8 encoding it might be more 
polite. Anyway, if any of the intl guys want to take issue with that, let's file 
another bug.

Comment 10

[Johnny Stenback (:jst)]

How about changing:

    char *cookie = nsString(aCookie).ToNewCString();

to

    char *cookie = ToNewCString(aCookie);

in ::SetCookie() to eliminate one extra malloc and string copy while we're at
it? Or we could just as easily make that ToNewUTF8String(aCookie) and make
::GetCookie() code use NS_ConvertUTF8toUCS2() (which does one extra unavoidable
string copy) to fix the unicode dataloss problems?

Comment 11

[Katsuhiko Momoi]

> Gratuitous copies aside, I'd have thought UTF-8 
> encoding it might be more polite. Anyway, if any of 
> the intl guys want to take issue with that, let's file 
> another bug.

I think the relevant RFC is the one above. The 'value'
according to this RFC could be anything the server wants
to set. But if we do any conversion on our side, how do we
make sure that the stored value matches that expected
by the server at the next visit? (Let's move this to 
another bug...)

Comment 12

[Paul Chen]

nav triage team:

Marking nsbeta1+ and p1.

Comment 13

[Christopher Blizzard (:blizzard)]

This ia 0.9.2 bug.  What's the deal here?

Comment 14

[Stephen P. Morse]

Awaiting for super-review.

Comment 15

[Chris Waterson]

Attachment: similar patch, fewer copies, still no UTF-8 support

Comment 16

[Stephen P. Morse]

r=morse for waterson's patch
still awaiting sr from scc.

Comment 17

[Scott Collins]

I like the patch, the only thing I might change is that I would try to manage
the life of the |char*| at the end automatically, e.g.,

  nsXPIDLCString cookie;
  cookie.Adopt(ToNewCString(aCookie));
  if ( cookie.get() )
    ...

I don't feel strongly enough about this to deny you an sr, but if this feels
right to you, go ahead and add it.  We're just punting on UTF8?

sr=scc

Comment 18

[chris hofmann]

a=chofmann branch and trunk

Comment 19

[Stephen P. Morse]

Fix checked in to both trunck and branch

Comment 20

[Tom Everingham]

verified: win2k 2001082303