1675587 - Unininitialized memory passed to connect() system call in nsSocketTranport2.cpp

Assignee

Description

•

4 years ago

•

During a local run of xpcshell tests of FULL DEBUG version of M-C/C-C Thunderbird under valgrind (I had to use --serialize and --verbose to see the output), I found that there are at least a couple of places where the address parameter to system call |connect| is not properly initialized.

The initial test run reported these uninitialized usages.

Output of
egrep  "Syscall param socketcall.connect" log1258-xpcshell-memcheck.txt:

21:50.91 pid:678268 ==678268== Syscall param socketcall.connect(serv_addr.sin6_scope_id) points to uninitialised byte(s)
58:21.06 pid:678875 ==678875== Syscall param socketcall.connect(serv_addr.sin6_scope_id) points to uninitialised byte(s)
109:57.10 pid:679723 ==679723== Syscall param socketcall.connect(serv_addr.sin6_scope_id) points to uninitialised byte(s)
150:34.65 pid:680335 ==680335== Syscall param socketcall.connect(serv_addr.sin6_scope_id) points to uninitialised byte(s)
34:37.24 pid:688519 ==688519== Syscall param socketcall.connect(serv_addr.sin6_scope_id) points to uninitialised byte(s)

These are triggered by the following tests:
I use a wrapper for local TB binary that reports the command parameters of valgrind invocation one by one, and from that
I learned these are the tests that triggered errors.

Output of
egrep  "const _TEST_NAME" log1258-xpcshell-memcheck.txt | egrep "(678268|678875|679723|680335|688519)" | grep finalargs &

20:35.05 pid:678268 finalargs[ 41] = const _TEST_NAME = "xpcshell-cpp.ini:comm/mailnews/compose/test/unit/test_mailTelemetry.js";
56:53.35 pid:678875 finalargs[ 41] = const _TEST_NAME = "xpcshell-cpp.ini:comm/mailnews/compose/test/unit/test_sendMessageLater3.js";
108:37.35 pid:679723 finalargs[ 41] = const _TEST_NAME = "xpcshell-jsm.ini:comm/mailnews/compose/test/unit/test_mailTelemetry.js";
148:45.65 pid:680335 finalargs[ 41] = const _TEST_NAME = "xpcshell-jsm.ini:comm/mailnews/compose/test/unit/test_sendMessageLater3.js";
23:28.88 pid:688519 finalargs[ 41] = const _TEST_NAME = "comm/mailnews/local/test/unit/test_over4GBMailboxes.js";

The uninitialized memory was created at the same locations. (Note that due to the local patches, the line number may differ from the current M-C.)

Output of
egrep -1  "Uninitialised value was" log1258-xpcshell-memcheck.txt

21:50.91 pid:678268 ==678268==	in frame #1, created by pt_Connect (ptio.c:1604)
21:50.91 pid:678268 ==678268==	Uninitialised value was created by a stack allocation
21:50.91 pid:678268 ==678268==	  at 0x62EC1B3: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1293)
--
58:21.07 pid:678875 ==678875==	in frame #1, created by pt_Connect (ptio.c:1604)
58:21.07 pid:678875 ==678875==	Uninitialised value was created by a stack allocation
58:21.07 pid:678875 ==678875==	  at 0x62EC1B3: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1293)
--
109:57.10 pid:679723 ==679723==	 in frame #1, created by pt_Connect (ptio.c:1604)
109:57.10 pid:679723 ==679723==	 Uninitialised value was created by a stack allocation
109:57.10 pid:679723 ==679723==	   at 0x62EC1B3: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1293)
--
150:34.65 pid:680335 ==680335==	 in frame #1, created by pt_Connect (ptio.c:1604)
150:34.65 pid:680335 ==680335==	 Uninitialised value was created by a stack allocation
150:34.65 pid:680335 ==680335==	   at 0x62EC1B3: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1293)
--
34:37.24 pid:688519 ==688519==	in frame #1, created by pt_Connect (ptio.c:1604)
34:37.24 pid:688519 ==688519==	Uninitialised value was created by a stack allocation
34:37.24 pid:688519 ==688519==	  at 0x62EC1B3: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1293)

The complete stack trace from another test run.
I invoke the test only for test_mailTelemetry.js

17:20.38 pid:702735 finalargs[ 41] = const _TEST_NAME = "xpcshell-jsm.ini:comm/mailnews/compose/test/unit/test_mailTelemetry.js";
...
19:50.61 pid:702735 ==702735== Thread 4 Socket Thread:
19:50.61 pid:702735 ==702735== Syscall param socketcall.connect(serv_addr.sin6_scope_id) points to uninitialised byte(s)
19:50.61 pid:702735 ==702735==	  at 0x487325B: connect (connect.c:26)
19:50.61 pid:702735 ==702735==	  by 0x12C98BD9: pt_Connect (ptio.c:1639)
19:50.61 pid:702735 ==702735==	  by 0x12C7CE86: PR_Connect (priometh.c:155)
19:50.61 pid:702735 ==702735==	  by 0x62E62A0: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1616)
19:50.61 pid:702735 ==702735==	  by 0x62E6C3A: mozilla::net::nsSocketTransport::OnSocketEvent(unsigned int, nsresult, nsISupports*) (nsSocketTransport2.cpp:2219)
19:50.61 pid:702735 ==702735==	  by 0x62E6FBE: mozilla::net::nsSocketEvent::Run() (nsSocketTransport2.cpp:93)
19:50.61 pid:702735 ==702735==	  by 0x60D2221: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:1197)
19:50.61 pid:702735 ==702735==	  by 0x60CDEEC: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:513)
19:50.61 pid:702735 ==702735==	  by 0x62C9193: mozilla::net::nsSocketTransportService::Run() (nsSocketTransportService2.cpp:1195)
19:50.61 pid:702735 ==702735==	  by 0x60D2221: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:1197)
19:50.61 pid:702735 ==702735==	  by 0x60CDEEC: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:513)
19:50.61 pid:702735 ==702735==	  by 0x693E47B: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (MessagePump.cpp:332)
19:50.61 pid:702735 ==702735==	  by 0x68DBAAE: MessageLoop::RunInternal() (message_loop.cc:334)
19:50.61 pid:702735 ==702735==	  by 0x68DBB0A: MessageLoop::RunHandler() (message_loop.cc:327)
19:50.61 pid:702735 ==702735==	  by 0x68DBFF3: MessageLoop::Run() (message_loop.cc:309)
19:50.61 pid:702735 ==702735==	  by 0x60ED7A0: nsThread::ThreadFunc(void*) (nsThread.cpp:442)
19:50.61 pid:702735 ==702735==	  by 0x12C9BD94: _pt_root (ptthread.c:201)
19:50.61 pid:702735 ==702735==	  by 0x4868EA6: start_thread (pthread_create.c:477)
19:50.61 pid:702735 ==702735==	  by 0x12B65EAE: clone (clone.S:95)
19:50.61 pid:702735 ==702735==	Address 0x162532f8 is on thread 4's stack
19:50.62 pid:702735 ==702735==	in frame #1, created by pt_Connect (ptio.c:1604)
19:50.62 pid:702735 ==702735==	Uninitialised value was created by a stack allocation
19:50.62 pid:702735 ==702735==	  at 0x62E5147: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1293)
19:50.62 pid:702735 ==702735==

The only parameter of connect that can have uninitialized memory is its second parameter from inspection. The third parameter is the length and is a fixed constant.

So I took care of the particular issue by clearing the address information to zero before setting up its various fields and eventually passing it to connect..

Clearing the address structure to zero is the typical usage pattern for BSD-ish network system call for a long time.
E.g.: Clearing address structure from man page of getaddrinfo(3) under linux.
Note the zeroing of a structure.

       ...
       int
       main(int argc, char *argv[])
       {
           struct addrinfo hints;
           struct addrinfo *result, *rp;
           int sfd, s;
           struct sockaddr_storage peer_addr;
           socklen_t peer_addr_len;
           ssize_t nread;
           char buf[BUF_SIZE];

           if (argc != 2) {
               fprintf(stderr, "Usage: %s port\n", argv[0]);
               exit(EXIT_FAILURE);
           }

--->       memset(&hints, 0, sizeof(struct addrinfo));
           hints.ai_family = AF_UNSPEC;    /* Allow IPv4 or IPv6 */
           hints.ai_socktype = SOCK_DGRAM; /* Datagram socket */
           hints.ai_flags = AI_PASSIVE;    /* For wildcard IP address */
           hints.ai_protocol = 0;          /* Any protocol */
           hints.ai_canonname = NULL;
           hints.ai_addr = NULL;
           hints.ai_next = NULL;
           ...

After the above problem was taken care of by zeroing the structure in one place, I noticed another uninitialized memory issue in another place.

Not sure why it was not flagged by the previous run, but
maybe, just maybe, the system call used to return somewhat different
value due to the uninitialized memory value and the program control
was changed for the better.

New memcheck:param signature for another uninitialized memory passed connect().

 2:08.35 pid:708145 finalargs[ 41] = const _TEST_NAME = "xpcshell-jsm.ini:comm/mailnews/compose/test/unit/test_mailTelemetry.js";
...

 3:30.56 pid:708145 ==708145== Thread 4 Socket Thread:
 3:30.56 pid:708145 ==708145== Syscall param socketcall.connect(serv_addr.sin6_scope_id) points to uninitialised byte(s)
 3:30.56 pid:708145 ==708145==	  at 0x487325B: connect (connect.c:26)
 3:30.56 pid:708145 ==708145==	  by 0x12C98BD9: pt_Connect (ptio.c:1639)
 3:30.56 pid:708145 ==708145==	  by 0x12C7CE86: PR_Connect (priometh.c:155)
 3:30.56 pid:708145 ==708145==	  by 0x62E62A6: mozilla::net::nsSocketTransport::InitiateSocket() (nsSocketTransport2.cpp:1616)
 3:30.56 pid:708145 ==708145==	  by 0x62E6C40: mozilla::net::nsSocketTransport::OnSocketEvent(unsigned int, nsresult, nsISupports*) (nsSocketTransport2.cpp:2219)
 3:30.56 pid:708145 ==708145==	  by 0x62E6FC4: mozilla::net::nsSocketEvent::Run() (nsSocketTransport2.cpp:93)
 3:30.56 pid:708145 ==708145==	  by 0x60D2221: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:1197)
 3:30.56 pid:708145 ==708145==	  by 0x60CDEEC: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:513)
 3:30.56 pid:708145 ==708145==	  by 0x62C9193: mozilla::net::nsSocketTransportService::Run() (nsSocketTransportService2.cpp:1195)
 3:30.56 pid:708145 ==708145==	  by 0x60D2221: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:1197)
 3:30.56 pid:708145 ==708145==	  by 0x60CDEEC: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:513)
 3:30.56 pid:708145 ==708145==	  by 0x693E481: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (MessagePump.cpp:332)
 3:30.56 pid:708145 ==708145==	  by 0x68DBAB4: MessageLoop::RunInternal() (message_loop.cc:334)
 3:30.56 pid:708145 ==708145==	  by 0x68DBB10: MessageLoop::RunHandler() (message_loop.cc:327)
 3:30.56 pid:708145 ==708145==	  by 0x68DBFF9: MessageLoop::Run() (message_loop.cc:309)
 3:30.56 pid:708145 ==708145==	  by 0x60ED7A0: nsThread::ThreadFunc(void*) (nsThread.cpp:442)
 3:30.56 pid:708145 ==708145==	  by 0x12C9BD94: _pt_root (ptthread.c:201)
 3:30.56 pid:708145 ==708145==	  by 0x4868EA6: start_thread (pthread_create.c:477)
 3:30.56 pid:708145 ==708145==	  by 0x12B65EAE: clone (clone.S:95)
 3:30.56 pid:708145 ==708145==	Address 0x162532f8 is on thread 4's stack
 3:30.56 pid:708145 ==708145==	in frame #1, created by pt_Connect (ptio.c:1604)
 3:30.56 pid:708145 ==708145==	Uninitialised value was created by a stack allocation
 3:30.56 pid:708145 ==708145==	  at 0x633BCA1: nsHostResolver::InitLoopbackRecord(nsHostKey const&, nsresult*) (nsHostResolver.cpp:946)
 3:30.56 pid:708145 ==708145==

Note that the uninitialized memory is created in another place.

So I zeroed the address structure there.

And all is well so far.

I am attaching a patch in another post.

ISHIKAWA, Chiaki

Assignee

Comment 1

•

4 years ago

Attached patch Initializing address structure to zero. — Details — Splinter Review

Attached is the patch to eliminate the uninitialized memory usage reported here.

Since I don't submit M-C patches for Firefox often and not familiar with
the current tools used for code submission, I wonder if someone can take this up
and land it in M-C.

Or if someone can handhole me, maybe I can do it, but I think this is a relatively straightforward issue and someone can do it in one go quickly.

Assignee: nobody → ishikawa

ISHIKAWA, Chiaki

Assignee

Comment 2

•

4 years ago

•

Edited

Oh, I see the submission of a patch automagically assign this bug to me.
So from whom should I request a review?
TIA

ISHIKAWA, Chiaki

Assignee

Updated

•

4 years ago

Flags: needinfo?(jstutte)

Jens Stutte [:jstutte]

Updated

•

4 years ago

Flags: needinfo?(jstutte) → needinfo?(valentin.gosu)

Valentin Gosu [:valentin] (he/him)

Comment 3

•

4 years ago

This is a great find, thanks!
Normally it's best if you use the moz-phab tool to upload the patch, but in this case I can do it for you.

Severity: -- → S3

Flags: needinfo?(valentin.gosu)

Priority: -- → P2

Whiteboard: [necko-triaged]

Valentin Gosu [:valentin] (he/him)

Comment 4

•

4 years ago

Attached file Bug 1675587 - clearing address structure passed to |connect()| r=#necko — Details

Pulsebot

Comment 5

•

4 years ago

Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/275771d26b9f
clearing address structure passed to |connect()| r=necko-reviewers,kershaw

Narcis Beleuzu [:NarcisB]

Comment 6

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/275771d26b9f

Status: NEW → RESOLVED

Closed: 4 years ago

status-firefox84: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 84 Branch

ISHIKAWA, Chiaki

Assignee

Comment 7

•

4 years ago

(In reply to Valentin Gosu [:valentin] (he/him) from comment #3)

This is a great find, thanks!
Normally it's best if you use the moz-phab tool to upload the patch, but in this case I can do it for you.

Thank you very much (!).

Valentin Gosu [:valentin] (he/him)

Updated

•

3 years ago

Updated

•

2 years ago

Blocks: 1670146

Initializing address structure to zero. 4 years ago ISHIKAWA, Chiaki 1.65 KB, patch		Details \| Diff \| Splinter Review
Bug 1675587 - clearing address structure passed to \|connect()\| r=#necko 4 years ago Valentin Gosu [:valentin] (he/him) 47 bytes, text/x-phabricator-request		Details \| Review

Bugzilla

Quick Search

Unininitialized memory passed to connect() system call in nsSocketTranport2.cpp

Categories

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

Tracking

()

People

(Reporter: ishikawa, Assigned: ishikawa)

References

Details

(Whiteboard: [necko-triaged])

Crash Data

Security

(public)

User Story

Attachments

(2 files)

Description

Comment 1

Comment 2

Updated

Updated

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Updated

Updated

Attachment

General

Description

File Name

Content Type