Bug 220668

Created attachment 417731 [details] Patch

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

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

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

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

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

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.

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.

Created attachment 417777 [details] Patch

Created attachment 417778 [details] Patch

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

Can Valgrind suppress the warning by memset after resize? attachmentInfo.resize(attachments.size()); memset(attachmentInfo.data(), sizeof(AttachmentInfo) * attachments.size(), 0);

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

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

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

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

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

Created attachment 417887 [details] Patch

Comment on attachment 417778 [details] Patch Sigh... accidentally unset r? by uploading a patch to the wrong bug. Let's try again....

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

Created attachment 417890 [details] Patch for landing

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

Created attachment 417917 [details] Patch

(In reply to Darin Adler from comment #28) > Neat, I like that. Please set cq+ if you're happy with this hopefully-final version.

Committed r271626: <https://trac.webkit.org/changeset/271626> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417917 [details].