Bug 220668 - Syscall param sendmsg(msg.msg_iov[1]) points to uninitialised byte(s) in IPC::Connection::sendOutgoingMessage
Summary: Syscall param sendmsg(msg.msg_iov[1]) points to uninitialised byte(s) in IPC:...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-15 13:48 PST by Michael Catanzaro
Modified: 2021-01-19 15:50 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.48 KB, patch)
2021-01-15 13:51 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (6.66 KB, patch)
2021-01-17 05:26 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.50 KB, patch)
2021-01-17 05:36 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.18 KB, patch)
2021-01-19 10:24 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (5.98 KB, patch)
2021-01-19 11:01 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (5.73 KB, patch)
2021-01-19 15:10 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-01-15 13:48:24 PST
This is a follow-up to bug #146729. We previously discovered that we need to either (a) manually zero the MessageInfo struct with memcpy (accepted solution), or b) use __attribute__((packed)) to omit struct packing. This seemed to fix our valgrind warnings when bmalloc was in use, but not when bmalloc is disabled, as is good practice when valgrinding.

Turns out we need to zero (or pack) the AttachmentInfo struct as well. Let's go with zeroing it, since it is a trivially-copyable type, so it's safe to do.
Comment 1 Michael Catanzaro 2021-01-15 13:51:34 PST
Created attachment 417731 [details]
Patch
Comment 2 Darin Adler 2021-01-15 13:57:33 PST
Comment on attachment 417731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417731&action=review

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:72
> +    AttachmentInfo()
> +    {
> +        // The entire AttachmentInfo is passed to write(), so we have to zero our
> +        // padding bytes to avoid writing uninitialized memory.
> +        memset(this, 0, sizeof(*this));
> +    }

Don’t we need this for the copy and move constructors too? They are compiler generated, we do not delete them, it seems unlikely they guarantee they zero out the padding bytes.
Comment 3 Michael Catanzaro 2021-01-15 14:31:14 PST
Comment on attachment 417731 [details]
Patch

Yup, good point.

In practice, the copy and move constructors are not used, but let's be robust about this....
Comment 4 Darin Adler 2021-01-15 14:59:16 PST
(In reply to Michael Catanzaro from comment #3)
> In practice, the copy and move constructors are not used, but let's be
> robust about this....

If we want to take advantage of the fact that they are not used, we could delete them instead of implementing them correctly.
Comment 5 Michael Catanzaro 2021-01-15 15:07:00 PST
(In reply to Darin Adler from comment #4)
> If we want to take advantage of the fact that they are not used, we could
> delete them instead of implementing them correctly.

OK. I'll do so for MessageInfo as well.
Comment 6 Michael Catanzaro 2021-01-15 15:17:34 PST
(In reply to Michael Catanzaro from comment #3)
> In practice, the copy and move constructors are not used

I'm wrong. They are used, though somehow not in a way that results in valgrind complaining.

I'll implement them.
Comment 7 Michael Catanzaro 2021-01-15 15:39:38 PST
(In reply to Michael Catanzaro from comment #6)
> I'm wrong. They are used, though somehow not in a way that results in
> valgrind complaining.
> 
> I'll implement them.

That failed because implementing the copy constructor means the type is no longer trivially-copyable, so we wind up with -Wclass-memaccess warnings when trying to memcpy it. The way to avoid those warnings is to cast the pointers to void* to indicate "I know what I'm doing," but at this point the code is getting a bit messy. Deleting the copy constructor has the same problem.

So... I arrive back at my original patch here, but the original patch is not fully satisfying because it doesn't fully prevent uninitialized struct padding. Maybe we should just pack the structs and be done with it? Or maybe I should remove the constructors and use static create functions instead? Let me think about this....
Comment 8 Darin Adler 2021-01-15 15:42:57 PST
I think the memcpy with void* cast (presumably a static_cast) is a completely logical partner to the use of memset in the default constructor, even though it’s a bit ugly.

The whole thing is irritating: nothing WebKit-specific here. I wonder how others solve this problem.
Comment 9 Michael Catanzaro 2021-01-17 05:17:03 PST
I'm going to attach two patches:

 (1) Using memcpy() to zero the struct, as before
 (2) Using __attribute__((packed)) instead

(1) is getting a little messy. I think I prefer (2). It's not standard C++, but this is a platform-specific file anyway. Since struct members will no longer be aligned, it should perform worse, but this is IPC code anyway so any impact should be dwarfed by the cost of IPC.
Comment 10 Michael Catanzaro 2021-01-17 05:26:18 PST
Created attachment 417777 [details]
Patch
Comment 11 Michael Catanzaro 2021-01-17 05:36:16 PST
Created attachment 417778 [details]
Patch
Comment 12 Fujii Hironori 2021-01-18 19:07:58 PST
Comment on attachment 417778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417778&action=review

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:90
>      size_t m_size { 0 };

This makes m_size unaligned.
How about an idea adding padding?

    size_t m_size { 0 };
    Attachment::Type m_type { Attachment::Uninitialized };
    bool m_isNull { false };
    char m_padding[sizeof(Attachment::Type) - sizeof(bool)] = {};
Comment 13 Fujii Hironori 2021-01-18 19:10:32 PST
Comment on attachment 417778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417778&action=review

>> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:90
>>      size_t m_size { 0 };
> 
> This makes m_size unaligned.
> How about an idea adding padding?
> 
>     size_t m_size { 0 };
>     Attachment::Type m_type { Attachment::Uninitialized };
>     bool m_isNull { false };
>     char m_padding[sizeof(Attachment::Type) - sizeof(bool)] = {};

Oops. I missed your comment 9.
Comment 14 Fujii Hironori 2021-01-18 19:12:53 PST
Can Valgrind suppress the warning by memset after resize?

  attachmentInfo.resize(attachments.size());
  memset(attachmentInfo.data(), sizeof(AttachmentInfo) * attachments.size(), 0);
Comment 15 Fujii Hironori 2021-01-18 19:15:54 PST
Comment on attachment 417777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417777&action=review

> Source/WebKit/ChangeLog:19
> +        the original fix to MessageInfo.

If this patch is to suppress the Valgrind warning, I think we don't need them.
Comment 16 Fujii Hironori 2021-01-18 20:01:39 PST
Comment on attachment 417777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417777&action=review

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:113
> +    // The AttachmentInfo will be copied using memcpy, so all members must be trivially copyable.

Why did you remove the initializer even though you don't remove that of MessageInfo?
Comment 17 Fujii Hironori 2021-01-18 20:45:13 PST
I don't like the unaligned access just for suppressing Valgrind warning.
I prefer (1). I'd like to remove other constructors if possible.
Comment 18 Michael Catanzaro 2021-01-19 06:17:59 PST
Comment on attachment 417777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417777&action=review

>> Source/WebKit/ChangeLog:19
>> +        the original fix to MessageInfo.
> 
> If this patch is to suppress the Valgrind warning, I think we don't need them.

The copy/move constructors are indeed not needed to suppress the valgrind warning, but Darin requested them for robustness to ensure a MessageInfo/AttachmentInfo cannot be created with uninitialized padding.

Then the copy/move assignment operators are required if we implement the constructors, but these can use = default since the padding is already initialized at this point.

>> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:113
>> +    // The AttachmentInfo will be copied using memcpy, so all members must be trivially copyable.
> 
> Why did you remove the initializer even though you don't remove that of MessageInfo?

An oversight, good catch. I should remove the initializers from MessageInfo as well, since they are useless when the whole struct is initialized to 0 with memcpy().
Comment 19 Michael Catanzaro 2021-01-19 06:18:18 PST
(In reply to Fujii Hironori from comment #14)
> Can Valgrind suppress the warning by memset after resize?
> 
>   attachmentInfo.resize(attachments.size());
>   memset(attachmentInfo.data(), sizeof(AttachmentInfo) * attachments.size(),
> 0);

I think it should work. Let me test.
Comment 20 Michael Catanzaro 2021-01-19 08:41:58 PST
(In reply to Michael Catanzaro from comment #19)
> (In reply to Fujii Hironori from comment #14)
> > Can Valgrind suppress the warning by memset after resize?
> > 
> >   attachmentInfo.resize(attachments.size());
> >   memset(attachmentInfo.data(), sizeof(AttachmentInfo) * attachments.size(),
> > 0);
> 
> I think it should work. Let me test.

Doesn't work, not sure why:

==74618== Syscall param sendmsg(msg.msg_iov[1]) points to uninitialised byte(s)
==74618==    at 0xD6BEECD: sendmsg (in /usr/lib64/libpthread-2.32.so)
==74618==    by 0x6594195: IPC::Connection::sendOutputMessage(IPC::UnixMessage&) (ConnectionUnix.cpp:520)
==74618==    by 0x6595DC4: IPC::Connection::sendOutgoingMessage(std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >) (ConnectionUnix.cpp:437)
==74618==    by 0x658808D: sendOutgoingMessages (Connection.cpp:911)
==74618==    by 0x658808D: IPC::Connection::sendOutgoingMessages() (Connection.cpp:896)
==74618==    by 0xB2C9672: operator() (Function.h:83)
==74618==    by 0xB2C9672: WTF::RunLoop::performWork() (RunLoop.cpp:128)
==74618==    by 0xB316B98: operator() (RunLoopGLib.cpp:80)
==74618==    by 0xB316B98: WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) (RunLoopGLib.cpp:82)
==74618==    by 0xB31747E: operator() (RunLoopGLib.cpp:53)
==74618==    by 0xB31747E: WTF::RunLoop::{lambda(_GSource*, int (*)(void*), void*)#1}::_FUN(_GSource*, int (*)(void*), void*) (RunLoopGLib.cpp:56)
==74618==    by 0x48A410F: g_main_dispatch (gmain.c:3325)
==74618==    by 0x48A5032: g_main_context_dispatch (gmain.c:4043)
==74618==    by 0x48A521E: g_main_context_iterate (gmain.c:4119)
==74618==    by 0x48A5676: g_main_loop_run (gmain.c:4317)
==74618==    by 0xB31759F: WTF::RunLoop::run() (RunLoopGLib.cpp:108)
==74618==  Address 0x1fa65eb5 is 5 bytes inside a block of size 256 alloc'd
==74618==    at 0x4839809: malloc (vg_replace_malloc.c:307)
==74618==    by 0xB31EDBA: bmalloc::DebugHeap::malloc(unsigned long, bmalloc::FailureAction) (DebugHeap.cpp:98)
==74618==    by 0x659409E: malloc (FastMalloc.h:240)
==74618==    by 0x659409E: allocateBuffer<(WTF::FailureAction)0> (Vector.h:301)
==74618==    by 0x659409E: reserveCapacity<(WTF::FailureAction)0> (Vector.h:1195)
==74618==    by 0x659409E: reserveCapacity<(WTF::FailureAction)0> (Vector.h:1185)
==74618==    by 0x659409E: expandCapacity<(WTF::FailureAction)0> (Vector.h:1056)
==74618==    by 0x659409E: resize (Vector.h:1102)
==74618==    by 0x659409E: IPC::Connection::sendOutputMessage(IPC::UnixMessage&) (ConnectionUnix.cpp:484)
==74618==    by 0x6595DC4: IPC::Connection::sendOutgoingMessage(std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >) (ConnectionUnix.cpp:437)
==74618==    by 0x658808D: sendOutgoingMessages (Connection.cpp:911)
==74618==    by 0x658808D: IPC::Connection::sendOutgoingMessages() (Connection.cpp:896)
==74618==    by 0xB2C9672: operator() (Function.h:83)
==74618==    by 0xB2C9672: WTF::RunLoop::performWork() (RunLoop.cpp:128)
==74618==    by 0xB316B98: operator() (RunLoopGLib.cpp:80)
==74618==    by 0xB316B98: WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) (RunLoopGLib.cpp:82)
==74618==    by 0xB31747E: operator() (RunLoopGLib.cpp:53)
==74618==    by 0xB31747E: WTF::RunLoop::{lambda(_GSource*, int (*)(void*), void*)#1}::_FUN(_GSource*, int (*)(void*), void*) (RunLoopGLib.cpp:56)
==74618==    by 0x48A410F: g_main_dispatch (gmain.c:3325)
==74618==    by 0x48A5032: g_main_context_dispatch (gmain.c:4043)
==74618==    by 0x48A521E: g_main_context_iterate (gmain.c:4119)
==74618==    by 0x48A5676: g_main_loop_run (gmain.c:4317)
==74618==
Comment 21 Michael Catanzaro 2021-01-19 08:51:54 PST
(In reply to Fujii Hironori from comment #12)
> Comment on attachment 417778 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417778&action=review
> 
> > Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:90
> >      size_t m_size { 0 };
> 
> This makes m_size unaligned.
> How about an idea adding padding?
> 
>     size_t m_size { 0 };
>     Attachment::Type m_type { Attachment::Uninitialized };
>     bool m_isNull { false };
>     char m_padding[sizeof(Attachment::Type) - sizeof(bool)] = {};

This actually does work. Clever. I don't like it very much compared to my other patches... but it does work.

If anyone else has a preference, please say so. Otherwise, let's go with option (1) I guess, since nobody has objected to it.
Comment 22 Darin Adler 2021-01-19 09:24:32 PST
Comment on attachment 417777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417777&action=review

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:80
> +    AttachmentInfo(const AttachmentInfo& info)
> +    {
> +        memset(static_cast<void*>(this), 0, sizeof(*this));
> +        m_type = info.m_type;
> +        m_isNull = info.m_isNull;
> +        m_size = info.m_size;
> +    }

This looks good to me.

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:82
> +    AttachmentInfo(AttachmentInfo&& info)

On reflection, I think we can omit this function entirely. As long as we don’t implement a move constructor it will use the copy constructor, which for this class is fine. There’s no requirement when moving that the original object is zeroed, just needs to be safe to destroy.

> Source/WebKit/Platform/IPC/unix/UnixMessage.h:59
> +    MessageInfo(MessageInfo&& info)

Same thought here. Don’t need this.

> Source/WebKit/Platform/IPC/unix/UnixMessage.h:69
> +    MessageInfo& operator=(const MessageInfo&) = default;
> +    MessageInfo& operator=(MessageInfo&&) = default;

Don’t need these.
Comment 23 Michael Catanzaro 2021-01-19 10:24:58 PST
Created attachment 417887 [details]
Patch
Comment 24 Michael Catanzaro 2021-01-19 10:25:56 PST
Comment on attachment 417778 [details]
Patch

Sigh... accidentally unset r? by uploading a patch to the wrong bug. Let's try again....
Comment 25 Michael Catanzaro 2021-01-19 10:59:03 PST
(In reply to Darin Adler from comment #22)
> > Source/WebKit/Platform/IPC/unix/UnixMessage.h:69
> > +    MessageInfo& operator=(const MessageInfo&) = default;
> > +    MessageInfo& operator=(MessageInfo&&) = default;
> 
> Don’t need these.

I was able to remove the move constructors.

The explicit default copy assignment operator is needed to avoid -Wdeprecated-copy:

DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:532:64: warning: implicitly-declared ‘constexpr IPC::MessageInfo& IPC::MessageInfo::operator=(const IPC::MessageInfo&)’ is deprecated [-Wdeprecated-copy]
  532 | #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value)
      |                                                                ^
../../Source/WebKit/Platform/IPC/unix/UnixMessage.h:91:25: note: in expansion of macro ‘WTFMove’
   91 |         m_messageInfo = WTFMove(other.m_messageInfo);
      |                         ^~~~~~~
In file included from ../../Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:33,
                 from DerivedSources/WebKit/unified-sources/UnifiedSource-3b989221-1.cpp:2:
../../Source/WebKit/Platform/IPC/unix/UnixMessage.h:51:5: note: because ‘IPC::MessageInfo’ has user-provided ‘IPC::MessageInfo::MessageInfo(const IPC::MessageInfo&)’
   51 |     MessageInfo(const MessageInfo& info)
      |     ^~~~~~~~~~~
Comment 26 Michael Catanzaro 2021-01-19 11:01:03 PST
Created attachment 417890 [details]
Patch for landing
Comment 27 Michael Catanzaro 2021-01-19 14:09:38 PST
Comment on attachment 417890 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=417890&action=review

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:80
> +    AttachmentInfo(const AttachmentInfo& info)
> +    {
> +        memset(static_cast<void*>(this), 0, sizeof(*this));
> +        m_type = info.m_type;
> +        m_isNull = info.m_isNull;
> +        m_size = info.m_size;
> +    }

I realized this could be simplified to:

memset(static_cast<void*>(this), 0, sizeof(*this));
*this = info;

so as to reuse the copy assignment operator in the copy constructor. That seems slightly nicer.
Comment 28 Darin Adler 2021-01-19 14:17:31 PST
Comment on attachment 417890 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=417890&action=review

>> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:80
>> +    }
> 
> I realized this could be simplified to:
> 
> memset(static_cast<void*>(this), 0, sizeof(*this));
> *this = info;
> 
> so as to reuse the copy assignment operator in the copy constructor. That seems slightly nicer.

Neat, I like that.
Comment 29 Michael Catanzaro 2021-01-19 15:10:36 PST
Created attachment 417917 [details]
Patch
Comment 30 Michael Catanzaro 2021-01-19 15:11:05 PST
(In reply to Darin Adler from comment #28)
> Neat, I like that.

Please set cq+ if you're happy with this hopefully-final version.
Comment 31 EWS 2021-01-19 15:50:43 PST
Committed r271626: <https://trac.webkit.org/changeset/271626>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417917 [details].