Unininitialized memory passed to connect() system call in nsSocketTranport2.cpp
Categories
(Core :: Networking: HTTP, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox84 | --- | fixed |
People
(Reporter: ishikawa, Assigned: ishikawa)
References
Details
(Whiteboard: [necko-triaged])
Attachments
(2 files)
1.65 KB,
patch
|
Details | Diff | Splinter Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
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.
Assignee | ||
Comment 1•4 years ago
|
||
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 | ||
Comment 2•4 years ago
•
|
||
Oh, I see the submission of a patch automagically assign this bug to me.
So from whom should I request a review?
TIA
Assignee | ||
Updated•4 years ago
|
Updated•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.
Comment 6•4 years ago
|
||
bugherder |
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 (!).
Updated•3 years ago
|
Description
•