Bug 220668 – Syscall param sendmsg(msg.msg_iov[1]) points to uninitialised byte(s) in IPC::Connection::sendOutgoingMessage

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 417731 [details]
Patch

[Darin Adler]

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.

[Michael Catanzaro]

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....

[Darin Adler]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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....

[Darin Adler]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 417777 [details]
Patch

[Michael Catanzaro]

Created attachment 417778 [details]
Patch

[Fujii Hironori]

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)] = {};

[Fujii Hironori]

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.

[Fujii Hironori]

Can Valgrind suppress the warning by memset after resize?

  attachmentInfo.resize(attachments.size());
  memset(attachmentInfo.data(), sizeof(AttachmentInfo) * attachments.size(), 0);

[Fujii Hironori]

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.

[Fujii Hironori]

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?

[Fujii Hironori]

I don't like the unaligned access just for suppressing Valgrind warning.
I prefer (1). I'd like to remove other constructors if possible.

[Michael Catanzaro]

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().

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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==

[Michael Catanzaro]

(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.

[Darin Adler]

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.

[Michael Catanzaro]

Created attachment 417887 [details]
Patch

[Michael Catanzaro]

Comment on attachment 417778 [details]
Patch

Sigh... accidentally unset r? by uploading a patch to the wrong bug. Let's try again....

[Michael Catanzaro]

(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)
      |     ^~~~~~~~~~~

[Michael Catanzaro]

Created attachment 417890 [details]
Patch for landing

[Michael Catanzaro]

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.

[Darin Adler]

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.

[Michael Catanzaro]

Created attachment 417917 [details]
Patch

[Michael Catanzaro]

(In reply to Darin Adler from comment #28)
> Neat, I like that.

Please set cq+ if you're happy with this hopefully-final version.

[EWS]

Committed r271626: <https://trac.webkit.org/changeset/271626>

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