494764 - Conditional jump or move depends on uninitialised value at nsNNTPProtocol::ParseURL()

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

m_newsAction variable is not set in constructor so it can be used uninitialized:

Valgrind output:

==22747== Conditional jump or move depends on uninitialised value(s)
==22747==    at 0x5812916: nsNNTPProtocol::ParseURL(nsIURI*, char**, char**, char**) (nsNNTPProtocol.cpp:1385)
==22747==    by 0x5814605: nsNNTPProtocol::Initialize(nsIURI*, nsIMsgWindow*) (nsNNTPProtocol.cpp:456)
==22747==    by 0x582712A: nsNntpIncomingServer::GetNntpConnection(nsIURI*, nsIMsgWindow*, nsINNTPProtocol**) (nsNntpIncomingServer.cpp:583)
==22747==    by 0x5827190: nsNntpIncomingServer::LoadNewsUrl(nsIURI*, nsIMsgWindow*, nsISupports*) (nsNntpIncomingServer.cpp:614)
==22747==    by 0x581AD75: nsNntpService::RunNewsUrl(nsIURI*, nsIMsgWindow*, nsISupports*) (nsNntpService.cpp:1267)
==22747==    by 0x581B381: nsNntpService::GetNewNews(nsINntpIncomingServer*, char const*, int, nsIUrlListener*, nsIMsgWindow*, nsIURI**) (nsNntpService.cpp:1298)
==22747==    by 0x581DBF3: nsMsgNewsFolder::GetNewsMessages(nsIMsgWindow*, int, nsIUrlListener*) (nsNewsFolder.cpp:895)
==22747==    by 0x581DCE3: nsMsgNewsFolder::GetNewMessages(nsIMsgWindow*, nsIUrlListener*) (nsNewsFolder.cpp:866)
==22747==    by 0x5821C4F: nsMsgNewsFolder::UpdateFolder(nsIMsgWindow*) (nsNewsFolder.cpp:350)
==22747==    by 0x41D285E: NS_InvokeByIndex_P (in /usr/lib/thunderbird-3.0b3pre/libxpcom_core.so)
==22747==    by 0x5D20DB9: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xpcwrappednative.cpp:2450)
==22747==    by 0x5D28DFB: XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, int*, int*) (xpcwrappednativejsops.cpp:1583)

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Initializes m_newsAction in nsNNTPProtocol::nsNNTPProtocol()

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 379554 [details] [diff] [review]
patch

Can you check it please?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Assigning to Stransky; since he was kind enough to provide a patch.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 379554 [details] [diff] [review]
patch

Switching reviewer to jcranmer, as he's the News module owner...

Comment 5

[Ludovic Hirlimann [:Usul]]

(In reply to comment #4)
> (From update of attachment 379554 [details] [diff] [review])
> Switching reviewer to jcranmer, as he's the News module owner...

Dan where can this bit of information be found ?

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 379554 [details] [diff] [review] [details])
> > Switching reviewer to jcranmer, as he's the News module owner...
> 
> Dan where can this bit of information be found ?

I've just added a link set here: https://developer.mozilla.org/en/comm-central#Requirements - mailnews specific rules apply to this patch.

Comment 7

[Joshua Cranmer [:jcranmer]]

Comment on attachment 379554 [details] [diff] [review]
patch

I don't think this is the right way to go--protocol objects can be reused, so if the value is not getting initialized, it will have the wrong value on the next URL being run.

A better option would be to move the initialization of m_newsAction (the proper one, m_runningUrl->GetNewsAction) before the call to ParseURL in Initialize.

Comment 8

[Jan Horak [:jhorak]]

Attachment: Patch v2: moved to Initialize method

Moving m_newsAction initialization into Initialize method. Please could you look into it?

Comment 9

[Ludovic Hirlimann [:Usul]]

Comment on attachment 404820 [details] [diff] [review]
Patch v2: moved to Initialize method


> https://developer.mozilla.org/en/comm-central#Requirements - mailnews specific
> rules apply to this patch.

sr is required - so requesting it.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 404820 [details] [diff] [review]
Patch v2: moved to Initialize method

Sorry for the delay.

In future it is useful if you could include -p in the diff options when creating the patch to submit - it gives an idea of which functions code is in, which makes it easier to review the patch.

sr=Standard8. Also a=Standard8 for checkin to the code base during the TB 3 lockdown period.

I'm assuming you don't have checkin privs, so I'll do this for you in a while.

Comment 11

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/3edac5d7d939