iox-#2414 Use placement new to construct a new object during assignment in 'variant' #2413

yuanxingyang · 2025-01-16T12:03:04Z

Notes for Reviewer

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Consider a second reviewer for complex new features or larger refactorings
Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes 'variant' has undefined behavior for assignment operator of underlying value #2414

elfenpiff · 2025-01-17T09:21:14Z

@yuanxingyang Thank you for your contribution!

Please follow the contribution rules: https://github.com/eclipse-iceoryx/iceoryx/blob/main/CONTRIBUTING.md

Sign the eclipse contributor agreement with the same account you committed
Create an issue first
Adjust every commit message so that it has the prefix iox-#??? where ??? is the issue number

When this is done I am happy to merge your contribution

elBoberido · 2025-01-17T14:22:04Z

@yuanxingyang the process is a bit cumbersome for an initial contributor. The reason for all the 'paperwork' is because we are an Eclipse project (therefore the ECA) and we aim for safety certification (therefore the issue for traceability).

To ease your first contribution, I created #2414 for you. Please use this issue number for your commit. You can change the commit message with git commit --amend. For more regular contributions, the git hooks can be used:
https://github.com/eclipse-iceoryx/iceoryx/blob/main/tools/git-hooks/Readme.md

If possible, it would be great to create a tests for the bugfix.

iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl

iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp

elBoberido · 2025-01-19T13:49:13Z

@yuanxingyang it seems some tests are now failing. I guess it's because they were based on assumption which don't hold anymore.

Oh, and please also consider adding your copyright to the files you think have non trivial changes.

yuanxingyang · 2025-01-19T15:54:11Z

@yuanxingyang it seems some tests are now failing. I guess it's because they were based on assumption which don't hold anymore.

Oh, and please also consider adding your copyright to the files you think have non trivial changes.

In the latest commit, I fixed some issues with the test cases. Since the implementation of EXPECT relies on variant, I also modified some test cases related to EXPECT. Please help review the latest test changes to see if they are reasonable

I am not considering adding my own copyright notice; the modified code is fully shared with everyone

iceoryx_hoofs/test/moduletests/test_vocabulary_expected.cpp

elBoberido · 2025-01-19T16:12:59Z

iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp

+            EXPECT_THAT(ignatz.index(), Eq(iox::INVALID_VARIANT_INDEX));
        }
        EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
        DTorTest::dtorWasCalled = false;
    }
-    EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
+    EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));


Okay, this leads to a semantic change in the variant. The moved from variant does not call the d'tor of the underlying value after it is moved. This should be fine though, although maybe unexpected.

@elfenpiff since you wrote the variant, your thoughts?

@elBoberido actually, this is not fine. C++ requires that the dtor is called after an object is moved otherwise we might have a leak here.

Objects must be always safely destructable after move. For instance when one has a threadsafe class, the contents of that class might be moved but since a mutex is unmovable, the mutex is still valid and is only cleaned up after the destructor is called.
This makes sense, since it is allowed to assign a moved class a new value. Simple example could be a threadsafe string. This thing could be moved, and it is legal to assign a completely new string to the moved version.

@yuanxingyang it seems you need to find another solution to the problem you want to fix.

Since the fix was so simple, we did not ask about the problem, which we should have done in the first place. Please excuse this.

Can you describe your issue? With more information we might find a solution that works for you and does not break the contract.

Thank you for your response. I agree with your explanation. Returning to my original question, I made the following modifications（the following code is the simplified version that can reproduce the issue）:
diff --git a/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp b/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp
index ac4d659d7..3e7a3e9a3 100644
--- a/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp
+++ b/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp
@@ -41,6 +41,8 @@ expected<IpcRuntimeInterface, IpcRuntimeInterfaceError> IpcRuntimeInterface::cre

MgmtShmCharacteristics mgmtShmCharacteristics; + iox::variant<IpcInterfaceUser> tmp; + tmp = IpcInterfaceUser(roudi::IPC_CHANNEL_ROUDI_NAME, domainId, ResourceType::ICEORYX_DEFINED);

When running ./iox-roudi and ./iox-cpp-publisher, the ./iox-cpp-publisher will crash. The backtrace is as follows:"

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from ./iox-cpp-publisher...
(gdb) run
Starting program: /home/ynx3sgh/work/projects/iceoryx/build/install/prefix/bin/iox-cpp-publisher
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
2025-01-21 16:41:44.770 [Warn ]: IPC channel still there, doing an unlink of 'iox1_0_u_iox-cpp-publisher'

Program received signal SIGSEGV, Segmentation fault.
0x00005555555ad587 in iox::internal::call_at_index<0ul, iox::runtime::IpcInterfaceUser>::destructor (index=0, ptr=0x7fffffec8810) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant_internal.hpp:177
177 reinterpret_cast<T*>(ptr)->~T();

(gdb) bt #0 0x00005555555ad587 in iox::internal::call_at_index<0ul, iox::runtime::IpcInterfaceUser>::destructor (index=0, ptr=0x7fffffec8810) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant_internal.hpp:177 #1 0x00005555555acad5 in iox::variant<iox::runtime::IpcInterfaceUser>::call_element_destructor (this=0x7fffffec8810) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl:141 #2 0x00005555555ac0e0 in iox::variant<iox::runtime::IpcInterfaceUser>::~variant (this=0x7fffffec8810, __in_chrg=<optimised out>) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl:133 #3 0x00005555555aaf40 in iox::runtime::IpcRuntimeInterface::create (runtimeName=..., domainId=..., roudiWaitingTimeout=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp:157 #4 0x000055555557f179 in operator() (__closure=0x7ffffff968f0) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime_impl.cpp:64 #5 0x000055555557f7d2 in iox::runtime::PoshRuntimeImpl::PoshRuntimeImpl (this=0x5555556768c0 <iox::runtime::PoshRuntime::defaultRuntimeFactory(iox::optional<iox::string<87ul> const*>)::buf>, name=..., domainId=..., location=iox::runtime::RuntimeLocation::SEPARATE_PROCESS_FROM_ROUDI) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime_impl.cpp:116 #6 0x000055555557cb6c in operator()<iox::optional<iox::string<87> const*> > (__closure=0x7fffffffd1cf, name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:87 #7 0x000055555557cc30 in iox::runtime::PoshRuntime::defaultRuntimeFactory (name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:90 #8 0x000055555557cda6 in iox::runtime::PoshRuntime::getInstance (name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:107 #9 0x000055555557cd35 in iox::runtime::PoshRuntime::initRuntime (name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:102 #10 0x00005555555613db in main () at /home/ynx3sgh/work/projects/iceoryx/iceoryx_examples/icedelivery/iox_publisher.cpp:37

(gdb) quit

I probably used the wrong method to assign a value to the variant. When I replaced = with emplace to assign a value, the crash no longer occurred.

@yuanxingyang I have the feeling that the rule of 5 was not followed and IpcInterfaceUser should not be cooyable at all. This is quite an old class, with it's origins at an R&D department. I'll have a look at it later on.

@yuanxingyang I had a closer look and as it turned out, the converting assignment operator is broken and I'm not sure quite sure if it can easily be fixed.

The problem is that currently the assumption is that if the type index matches the type of the value which is assigned, the variant already contains a valid value and can be assigned. In your case, the variant does not yet contain a valid value and the assignment operator is called on an uninitialized value. This leads to the issue you described.

I think one needs to have a close look on the semantics of the std::variant and then closely mimic that behavior. For example, the std::variant does not allow to use the conversion operator if it contains the same type twice.

The problem is that something like this also does not work universally, since emplace requires the availability of the move ctor.

if (!has_bad_variant_element_access<T>()) { auto storage = reinterpret_cast<T*>(&m_storage); *storage = std::forward<T>(rhs); } else { emplace<T>(std::forward<T>(rhs)); }

If you have an idea, feel free to share it.

hello @elBoberido
The latest commit align the behavior with std::variant. The reason is that reinterpret_cast is intended for use on already existing objects, so for creating a new object for the first time, placement new is used

iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp

codecov · 2025-01-19T20:48:14Z

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.66%. Comparing base (f33d582) to head (60008d5).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...yx_hoofs/vocabulary/include/iox/detail/variant.inl	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2413      +/-   ##
==========================================
- Coverage   78.68%   78.66%   -0.02%     
==========================================
  Files         440      440              
  Lines       16986    16983       -3     
  Branches     2361     2360       -1     
==========================================
- Hits        13365    13360       -5     
- Misses       2740     2742       +2     
  Partials      881      881

Flag	Coverage Δ
unittests	`78.46% <66.66%> (-0.02%)`	⬇️
unittests_timing	`15.35% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...yx_hoofs/vocabulary/include/iox/detail/variant.inl	`92.00% <66.66%> (-2.18%)`	⬇️

... and 1 file with indirect coverage changes

elBoberido

Sorry for letting you wait so long. This totally slipped my attention.

elBoberido · 2025-02-18T18:03:54Z

iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl

+        if (m_type_index != INVALID_VARIANT_INDEX)
+        {


Isn't this if-statement superfluous?

iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp

elBoberido

@yuanxingyang thanks for you patience. Looks good now. I just want to double check with @elfenpiff if he also agrees with the solution.

elBoberido · 2025-02-19T16:17:46Z

@yuanxingyang can you please fix the clang-tidy warning

…during assignment

yuanxingyang · 2025-02-20T08:21:35Z

@yuanxingyang can you please fix the clang-tidy warning
OK， I'll try to fix it

elBoberido · 2025-02-20T09:08:51Z

iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl

+        call_element_destructor();
+        // NOLINTJUSTIFICATION clang-tidy suggests that std::move(rhs) might erroneously move an lvalue, but here rhs is already T&&, so it's safe.
+        // NOLINTNEXTLINE(bugprone-move-forwarding-reference)
+        new (&m_storage) T(std::move(rhs));


@yuanxingyang Does the suggestion from clang-tidy to use std::forward not work?

@yuanxingyang Does the suggestion from clang-tidy to use std::forward not work?

It does not work; the following error occurs during compilation:

`In file included from /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/variant.hpp:331, from /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp:18: /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl: In instantiation of ‘typename std::enable_if<(! std::is_same<T, iox::variant<Types>&>::value), iox::variant<Types> >::type& iox::variant<Types>::operator=(T&&) [with T = int; Types = {int, float}; typename std::enable_if<(! std::is_same<T, iox::variant<Types>&>::value), iox::variant<Types> >::type = iox::variant<int, float>]’: /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp:466:15: required from here /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl:157:40: error: no matching function for call to ‘forward(int&)’ 157 | new (&m_storage) T(std::forward(rhs)); `

@yuanxingyang move cannot be used in this context. This operator is used for copy assignment and it would be a bug if a complex type would be assigned to a default constructed variant.

Unfortunately, our tests currently do not cover this scenario yet.

elBoberido added the bugfix Solves a bug label Jan 17, 2025

elBoberido reviewed Jan 17, 2025

View reviewed changes

iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl Outdated Show resolved Hide resolved

yuanxingyang changed the title ~~Reset index after move construction~~ iox-#2414 Reset index after move construction Jan 17, 2025

yuanxingyang force-pushed the main branch from 70d1817 to 9d4b3ca Compare January 18, 2025 16:15

elBoberido reviewed Jan 18, 2025

View reviewed changes

iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp Outdated Show resolved Hide resolved

yuanxingyang force-pushed the main branch from 9d4b3ca to 83c4f3c Compare January 19, 2025 04:58

yuanxingyang force-pushed the main branch from 83c4f3c to 6bcfa98 Compare January 19, 2025 15:47

elBoberido reviewed Jan 19, 2025

View reviewed changes

yuanxingyang force-pushed the main branch from 6bcfa98 to 963b232 Compare January 19, 2025 16:44

yuanxingyang force-pushed the main branch 2 times, most recently from 9513ee4 to 7673b3b Compare January 26, 2025 00:13

elBoberido reviewed Feb 18, 2025

View reviewed changes

yuanxingyang force-pushed the main branch from 7673b3b to b68079b Compare February 19, 2025 02:59

elBoberido reviewed Feb 19, 2025

View reviewed changes

elBoberido changed the title ~~iox-#2414 Reset index after move construction~~ iox-#2414 Use placement new to construct a new object during assignment in 'variant' Feb 19, 2025

iox-eclipse-iceoryx#2414 Use placement new to construct a new object …

60008d5

…during assignment

yuanxingyang force-pushed the main branch from b68079b to 60008d5 Compare February 20, 2025 08:09

elBoberido reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#2414 Use placement new to construct a new object during assignment in 'variant' #2413

iox-#2414 Use placement new to construct a new object during assignment in 'variant' #2413

yuanxingyang commented Jan 16, 2025 •

edited by elBoberido

Loading

elfenpiff commented Jan 17, 2025

elBoberido commented Jan 17, 2025

elBoberido commented Jan 19, 2025 •

edited

Loading

yuanxingyang commented Jan 19, 2025

elBoberido Jan 19, 2025

elfenpiff Jan 20, 2025

elBoberido Jan 20, 2025

yuanxingyang Jan 21, 2025 •

edited

Loading

yuanxingyang Jan 21, 2025

elBoberido Jan 21, 2025

elBoberido Jan 22, 2025

yuanxingyang Jan 26, 2025

codecov bot commented Jan 19, 2025 •

edited

Loading

elBoberido left a comment

elBoberido Feb 18, 2025

elBoberido left a comment

elBoberido commented Feb 19, 2025

yuanxingyang commented Feb 20, 2025

elBoberido Feb 20, 2025

yuanxingyang Feb 20, 2025 •

edited

Loading

elBoberido Feb 24, 2025

iox-#2414 Use placement new to construct a new object during assignment in 'variant' #2413

Are you sure you want to change the base?

iox-#2414 Use placement new to construct a new object during assignment in 'variant' #2413

Conversation

yuanxingyang commented Jan 16, 2025 • edited by elBoberido Loading

Notes for Reviewer

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

elfenpiff commented Jan 17, 2025

elBoberido commented Jan 17, 2025

elBoberido commented Jan 19, 2025 • edited Loading

yuanxingyang commented Jan 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanxingyang Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2025 • edited Loading

Codecov Report

elBoberido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido commented Feb 19, 2025

yuanxingyang commented Feb 20, 2025

Choose a reason for hiding this comment

yuanxingyang Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanxingyang commented Jan 16, 2025 •

edited by elBoberido

Loading

elBoberido commented Jan 19, 2025 •

edited

Loading

yuanxingyang Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 19, 2025 •

edited

Loading

yuanxingyang Feb 20, 2025 •

edited

Loading