Last Comment Bug 494764 - Conditional jump or move depends on uninitialised value at nsNNTPProtocol::ParseURL()
: Conditional jump or move depends on uninitialised value at nsNNTPProtocol::Pa...
Status: RESOLVED FIXED
: fixed-seamonkey2.0.1
Product: MailNews Core
Classification: Components
Component: Networking: NNTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 3.0rc1
Assigned To: Jan Horak
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-25 05:01 PDT by Martin Stránský
Modified: 2009-12-13 06:05 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (983 bytes, patch)
2009-05-25 05:28 PDT, Martin Stránský
Pidgeot18: review-
Details | Diff | Splinter Review
Patch v2: moved to Initialize method (1.21 KB, patch)
2009-10-06 06:48 PDT, Jan Horak
Pidgeot18: review+
standard8: superreview+
standard8: approval‑thunderbird3+
Details | Diff | Splinter Review

Description Martin Stránský 2009-05-25 05:01:23 PDT
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ý 2009-05-25 05:28:29 PDT
Created attachment 379554 [details] [diff] [review]
patch

Initializes m_newsAction in nsNNTPProtocol::nsNNTPProtocol()
Comment 2 Martin Stránský 2009-05-25 05:31:37 PDT
Comment on attachment 379554 [details] [diff] [review]
patch

Can you check it please?
Comment 3 Dan Mosedale (:dmose) 2009-05-26 11:37:06 PDT
Assigning to Stransky; since he was kind enough to provide a patch.
Comment 4 Dan Mosedale (:dmose) 2009-05-26 11:37:57 PDT
Comment on attachment 379554 [details] [diff] [review]
patch

Switching reviewer to jcranmer, as he's the News module owner...
Comment 5 Ludovic Hirlimann [:Usul] 2009-05-27 02:20:33 PDT
(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) 2009-05-27 03:08:24 PDT
(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] 2009-06-03 17:49:18 PDT
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 2009-10-06 06:48:29 PDT
Created attachment 404820 [details] [diff] [review]
Patch v2: moved to Initialize method

Moving m_newsAction initialization into Initialize method. Please could you look into it?
Comment 9 Ludovic Hirlimann [:Usul] 2009-10-07 00:07:45 PDT
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) 2009-10-16 06:17:45 PDT
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) 2009-10-16 06:24:55 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/3edac5d7d939

Note You need to log in before you can comment on or make changes to this bug.