531425 - Firefox 3.6b4 Crash [@ nsAuthSSPI::GetNextToken(void const*, unsigned int, void**, unsigned int*) ]

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[chris hofmann]

there is a new crash that shows up in beta4, not previously seen.  could be a regression or could be a morf of the signature name.

http://people.mozilla.com/~chofmann/crash-data/new-crashes/new-crashes-36b4-from-36b1-4-35all.html

signature looks like

https://crash-stats.mozilla.com/report/index/70d5e3de-8847-4f74-a423-458152091127

Frame  	Module  	Signature [Expand]  	Source
0 	xul.dll 	nsAuthSSPI::GetNextToken 	extensions/auth/nsAuthSSPI.cpp:372
1 	xul.dll 	nsHttpAuthCache::LookupAuthNode 	netwerk/protocol/http/src/nsHttpAuthCache.cpp:222


more reports at 

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsAuthSSPI::GetNextToken%28void%20const*,%20unsigned%20int,%20void**,%20unsigned%20int*%29&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsAuthSSPI::GetNextToken%28void%20const*,%20unsigned%20int,%20void**,%20unsigned%20int*%29&page=1

changes around the source file at the top of the stack indicates possible regression to investigate

396a38c7a6ae
2009-11-19 16:18 -0600	Jim Mathies - Bug 520607 - Remove use of "ntlm" auth module and replace with use of "sys-ntlm". r=wtc, cbiesinger. sr=bz.

Comment 1

[chris hofmann]

Comments might suggest a work around or maybe STR.  dirty profile?

> I installed 3.6b4 alongside 3.5.5, in /opt on SuSE 11.2 This crash occurs when I run /opt/firefox/firefox as a regular user. It doesn't happen if I run as root. Restarting also fails.

> This happens when I start FF 3.6b4 with my exisiting ~/.mozilla directory in place. It is OK if I remove that directory

Comment 2

[Jim Mathies [:jimm]]

(In reply to comment #1)
> Comments might suggest a work around or maybe STR.  dirty profile?
> 
> > I installed 3.6b4 alongside 3.5.5, in /opt on SuSE 11.2 This crash occurs when I run /opt/firefox/firefox as a regular user. It doesn't happen if I run as root. Restarting also fails.
> 
> > This happens when I start FF 3.6b4 with my exisiting ~/.mozilla directory in place. It is OK if I remove that directory

Chris, these comment don't seem to match to this crash?

Comment 3

[chris hofmann]

yeah,  sorry for the confusion.  comment 1 belongs in Bug 531305

Comment 4

[Jim Mathies [:jimm]]

Attachment: cache creds patch.v1

Best guess - these credential strings need to be valid for the duration of the connection. The auth cache pointers may not stay valid, so we keep a local copy in the auth module.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Can't you just use an nsString for the password too?  If you feel strongly about the SecureZeroMemory thing, you could do that using BeginWriting on the string, no?

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> Can't you just use an nsString for the password too?  If you feel strongly
> about the SecureZeroMemory thing, you could do that using BeginWriting on the
> string, no?

I guess I'm being too paranoid. I'd much rather just use an nsString. Another patch coming up.

Comment 7

[Jim Mathies [:jimm]]

Attachment: cache creds patch.v2

updated to comments.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 415485 [details] [diff] [review]
cache creds patch.v2

Make those reinterpret_cast, and looks good.

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #8)
> (From update of attachment 415485 [details] [diff] [review])
> Make those reinterpret_cast, and looks good.

I need the const_cast to ditch the const on nsAString's internal char_type.

Comment 10

[Boris Zbarsky [:bzbarsky]]

In that case, do a separate reinterpret_cast to const unsigned short* and then a const_cast to the non-const type?

And are we sure the callee won't modify this no-longer-const buffer?

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #10)
> In that case, do a separate reinterpret_cast to const unsigned short* and then
> a const_cast to the non-const type?
> 
> And are we sure the callee won't modify this no-longer-const buffer?

I can't see any reason why tit would. The api docs don't indicate any change will occur.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  We're not sharing these strings with anyone anyway, so that should be ok.

Comment 13

[Jim Mathies [:jimm]]

The tree is looking pretty busted at the moment, will check this in in the am.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

So why not use BeginWriting() instead of get()+const_cast?

Comment 15

[Jim Mathies [:jimm]]

(In reply to comment #14)
> So why not use BeginWriting() instead of get()+const_cast?

Looks like BeginWriting returns the same type - char_type*.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Correct. It avoids the const_cast.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yeah, I'd prefer the BeginWriting() too.  That would make it clear that we're not ensuring immutability anymore.

Comment 18

[Jim Mathies [:jimm]]

Attachment: cache creds patch.v3

Updated, kind of strange using a method named "BeginWriting" when I'm not writing anything, but such is life I guess with our string classes.

Comment 19

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/e58a42937197

Comment 20

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/48022d350200