1699024 - Uninitialized value usage in nsHttpTransaction.cpp. mConnection->BytesWritten() is bogus.

Status: RESOLVED FIXED

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

Description

[ISHIKAWA, Chiaki]

Attachment: valgrind-nsHttpConnection-mTotalBytesWriten.patch

Uninitialized value usage in nsHttpTransaction.cpp. mConnection->BytesWritten() is bogus.

While running xpcshell tests of thunderbird (TB) mail client under valgrind,
I saw errors related to uninitialized value usage.
One of them is reported here.

Valgrind error signature is as follows.
(Actually, the errors caused by the same reason was reported a few times.)

54:46.91 pid:53293 ==53293== Thread 4 Socket Thread:
54:46.91 pid:53293 ==53293== Conditional jump or move depends on uninitialised value(s)
54:46.91 pid:53293 ==53293==	at 0x62A7252: mozilla::net::nsHttpTransaction::Close(nsresult) (nsHttpTransaction.cpp:1514)
54:46.91 pid:53293 ==53293==	by 0x623F38A: mozilla::net::nsHttpConnection::CloseTransaction(mozilla::net::nsAHttpTransaction*, nsresult, bool) (nsHttpConnection.cpp:1783)
54:46.91 pid:53293 ==53293==	by 0x623FBDB: mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*) (nsHttpConnection.cpp:2458)
54:46.91 pid:53293 ==53293==	by 0x5D9A280: mozilla::net::nsSocketInputStream::OnSocketReady(nsresult) (nsSocketTransport2.cpp:287)
54:46.91 pid:53293 ==53293==	by 0x5DB35E4: mozilla::net::nsSocketTransport::OnSocketReady(PRFileDesc*, short) (nsSocketTransport2.cpp:2080)
54:46.91 pid:53293 ==53293==	by 0x5DA0170: mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) (nsSocketTransportService2.cpp:1389)
54:46.91 pid:53293 ==53293==	by 0x5DB468F: mozilla::net::nsSocketTransportService::Run() (nsSocketTransportService2.cpp:1160)
54:46.91 pid:53293 ==53293==	by 0x5BC5469: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:1152)
54:46.91 pid:53293 ==53293==	by 0x5BA5C0E: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:548)
54:46.91 pid:53293 ==53293==	by 0x6462C12: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (MessagePump.cpp:302)
54:46.91 pid:53293 ==53293==	by 0x63F43F8: MessageLoop::RunInternal() (message_loop.cc:335)
54:46.91 pid:53293 ==53293==	by 0x63F4454: MessageLoop::RunHandler() (message_loop.cc:328)
54:46.92 pid:53293 ==53293==	by 0x63F47B7: MessageLoop::Run() (message_loop.cc:310)
54:46.92 pid:53293 ==53293==	by 0x5BC8D0A: nsThread::ThreadFunc(void*) (nsThread.cpp:391)
54:46.92 pid:53293 ==53293==	by 0x146E7D94: _pt_root (ptthread.c:201)
54:46.92 pid:53293 ==53293==	by 0x486CEA6: start_thread (pthread_create.c:477)
54:46.92 pid:53293 ==53293==	by 0x14582DEE: clone (clone.S:95)
54:46.92 pid:53293 ==53293==  Uninitialised value was created by a heap allocation
54:46.92 pid:53293 ==53293==	at 0x483877F: malloc (vg_replace_malloc.c:307)
54:46.92 pid:53293 ==53293==	by 0x114458: moz_xmalloc (mozalloc.cpp:52)
54:46.92 pid:53293 ==53293==	by 0x617CFCF: mozilla::net::DnsAndConnectSocket::TransportSetup::SetupConn(mozilla::net::nsAHttpTransaction*, mozilla::net::ConnectionEntry*, nsresult, unsigned int, mozilla::net::HttpConnectionBase**) (cxxalloc.h:33)
54:46.92 pid:53293 ==53293==	by 0x617D8C3: mozilla::net::DnsAndConnectSocket::SetupConn(bool, nsresult) (DnsAndConnectSocket.cpp:492)
54:46.92 pid:53293 ==53293==	by 0x617E74A: mozilla::net::DnsAndConnectSocket::OnOutputStreamReady(nsIAsyncOutputStream*) (DnsAndConnectSocket.cpp:478)
54:46.92 pid:53293 ==53293==	by 0x5D9A4C6: mozilla::net::nsSocketOutputStream::OnSocketReady(nsresult) (nsSocketTransport2.cpp:519)
54:46.92 pid:53293 ==53293==	by 0x5DB35CA: mozilla::net::nsSocketTransport::OnSocketReady(PRFileDesc*, short) (nsSocketTransport2.cpp:2073)
54:46.92 pid:53293 ==53293==	by 0x5DA0170: mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) (nsSocketTransportService2.cpp:1389)
54:46.92 pid:53293 ==53293==	by 0x5DB468F: mozilla::net::nsSocketTransportService::Run() (nsSocketTransportService2.cpp:1160)
54:46.92 pid:53293 ==53293==	by 0x5BC5469: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:1152)
54:46.92 pid:53293 ==53293==	by 0x5BA5C0E: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:548)
54:46.92 pid:53293 ==53293==	by 0x6462C12: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (MessagePump.cpp:302)
54:46.92 pid:53293 ==53293==	by 0x63F43F8: MessageLoop::RunInternal() (message_loop.cc:335)
54:46.92 pid:53293 ==53293==	by 0x63F4454: MessageLoop::RunHandler() (message_loop.cc:328)
54:46.92 pid:53293 ==53293==	by 0x63F47B7: MessageLoop::Run() (message_loop.cc:310)
54:46.92 pid:53293 ==53293==	by 0x5BC8D0A: nsThread::ThreadFunc(void*) (nsThread.cpp:391)
54:46.92 pid:53293 ==53293==	by 0x146E7D94: _pt_root (ptthread.c:201)
54:46.92 pid:53293 ==53293==	by 0x486CEA6: start_thread (pthread_create.c:477)
54:46.92 pid:53293 ==53293==	by 0x14582DEE: clone (clone.S:95)

The following code trigger the usage of uninialized value.
https://searchfox.org/comm-central/source/mozilla/netwerk/protocol/http/nsHttpTransaction.cpp#1514

 // reallySentData is meant to separate the instances where data has
 // been sent by this transaction but buffered at a higher level while
 // a TLS session (perhaps via a tunnel) is setup.
 bool reallySentData =
     mSentData && (!mConnection || mConnection->BytesWritten());

After poking around a bit, I realize that mConnectin->BytesWriten() is returning
an uninitialized value.

I think we are not initializing the value referenced by the code properly.
I wonder how it got away from FF valgrind run's detection so far.

Probably mTotalBytesWritten = 0 ought to be in the initialization in the following code.
https://searchfox.org/comm-central/source/mozilla/netwerk/protocol/http/nsHttpConnection.cpp#69

nsHttpConnection::nsHttpConnection()
    : mSocketInCondition(NS_ERROR_NOT_INITIALIZED),
      mSocketOutCondition(NS_ERROR_NOT_INITIALIZED),
      mHttpHandler(gHttpHandler),
      mLastReadTime(0),
      mLastWriteTime(0),
      mMaxHangTime(0),
      mConsiderReusedAfterInterval(0),
      mConsiderReusedAfterEpoch(0),
      mCurrentBytesRead(0),
      mMaxBytesRead(0),
      mTotalBytesRead(0),    <---- mTotalBytesRead is initialized, but not for mTotalBytesWritten
      mContentBytesWritten(0),
      mUrgentStartPreferred(false),
      mUrgentStartPreferredKnown(false),
      mConnectedTransport(false),

Indeed, with the attached patch which initializes mTotalBytesWritten to zero, the error
disappears in local test, but I am not sure if that is the correct fix.
There are mention of connection reuse, etc., but only mTotalBytesRead is often referenced in such sections of code,
but not mTotalBytesWritten.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: Initialize mTotalBytesWritten

Comment 2

[ISHIKAWA, Chiaki]

I see that the patch in comment 1 fixes the issue in a header file declaration.
But then why not initialize, say, mTotalBytesRead (<- I meant Read not written...) in a same manner in the header file?
This is going to bring in a long-term maintenance issue unless there is a good reason to
initialize the value in the header file. (Well, if mTotalBytesWritten is initialized in header file, I would rather see all those fields/variables that can be done so follow suit. I have seen some variables/fields uninitialized and used for quite some time in mozilla code before.)

But again, I am a total newbie to the code in question. So there may be a good reason to do so.

Anyway, it is a total mystery to me why this has escaped the detection of valgrind so long.

Comment 3

[ISHIKAWA, Chiaki]

Anyway, it is a total mystery to me why this has escaped the detection of valgrind so long.

It seems that the use of mTotalBytesWritten has landed quite recently (about two weeks ago?). Then it figures.

Comment 4

[Dragana Damjanovic [:dragana]]

I want to move all initialization to be as mTotalBytesWritten at some point. And recently I started to initialize any new variables in this way.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f757d9a92c8
Initialize mTotalBytesWritten r=necko-reviewers,valentin

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6f757d9a92c8

Comment 7

[ISHIKAWA, Chiaki]

(In reply to Dragana Damjanovic [:dragana] from comment #4)

I want to move all initialization to be as mTotalBytesWritten at some point. And recently I started to initialize any new variables in this way.

I see. As long as you are on top of the issue, I am fine.
Thank you for taking care of the issue quickly.
Happy Hacking!