Update security branch to current master #424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Erik Boasson <eb@ilities.com>
Inspired by Coverity warnings. Signed-off-by: Erik Boasson <eb@ilities.com>
* Fix some typos. Signed-off-by: ChenYing Kuo <evshary@gmail.com> * Also update q_config.c, cyclonedds.rnc, cyclonedds.xsd for correct build. Signed-off-by: ChenYing Kuo <evshary@gmail.com> * Remove cdds.md. Signed-off-by: ChenYing Kuo <evshary@gmail.com>
the destination cache of the network stack is in a certain state. The issue is resolved by binding unicast sockets (incoming unicast and all outgoing traffic) to the address of the interface instead of inaddr_any (0.0.0.0). Set the new configuration option internal/BindUnicastToInterfaceAddr to false to get the old behavior. Co-authored-by: Erik Boasson <eb@ilities.com> Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
gcc 5.4 correctly warned that a null pointer was being passed into the entity-specific "set_qos" function when changing a topic QoS, where that parameter was tagged as "non-null". As it was never dereferenced in this case the resulting behaviour was still correct. It turns out that the entire function was overly complicated and that simply passing the entity pointer round allows eliminating a few arguments as well. (Oddly none of the more modern toolchains used pick this up.) Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
```
/opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/tools/pubsub/common.c:586:28: warning: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-int-float-conversion]
if(nanosec > nextafter(INT64_MAX, 0)) {
~~~~~~~~~ ^~~~~~~~~
/usr/include/stdint.h:134:22: note: expanded from macro 'INT64_MAX'
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/stdint.h:116:24: note: expanded from macro '__INT64_C'
^~~~~~
<scratch space>:345:1: note: expanded from here
9223372036854775807L
^~~~~~~~~~~~~~~~~~~~
1 warning generated.
```
Signed-off-by: Dan Rose <dan@digilabs.io>
The thread_states array resets the "state" to ZERO on thread termination to indicate that the slot was unused, but it leaves the thread id unchanged because some platforms don't have a defined value that will never be used as a thread id. A consequence is that a newly created thread may result in multiple slots containing their own thread id, but generally there will only be one that is not in state ZERO. However, the code for create_thread used to set the state to ALIVE prior to creating the thread, and so if the events get scheduled like: 1. thread A: X.state = ALIVE 2. create new thread B, storing tid in X.tid 3. thread A: Y.state = ALIVE 4. new thread B: lookup self (and cache pointer) 5. create new thread C, storing tid in Y.tid 6. new thread C: lookup self (and cache pointer) Then B will observe two slots in the ALIVE state, with X.tid certain to match and Y.tid undefined (and hence possibly matching). It may therefore pick Y. C will (in this schedule) of course always choose Y. They cache the pointer and never look at X and Y again, except for updating their virtual clocks. These virtual clocks are updated non-atomically (by design it is private to the thread) and so if both B & C use Y they can end up racing each other in updating the virtual clock and cause the nesting level of the "awake" state controlling garbage collection to get stuck (or wrap around, or do other horrible things). The consequence can be anything, from a somewhat benign variant where GC effectively stops and some operations (deleting readers and writers and shutting down) block forever, to use-after-free and the undefined behaviour that implies. This commit avoids looking up the slot in the newly created threads, instead passing the correct address in the argument. It also adds an intermediate state INIT that serves to reserve the slot until the new thread is actually running. It does make the look-up safe (if one were to do it), and as it is essentially free and gives more insight in the state of the system when viewed from a debugger, it appears a useful addition. Signed-off-by: Erik Boasson <eb@ilities.com>
* Don't pass null to memcmp ``` SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/ros/master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/serdes.hpp:135:3 in /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:41:15: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:41:15 in /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:41:31: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:41:31 in /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:45:15: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:45:15 in /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:45:30: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsi/src/ddsi_sertopic_default.c:45:30 in ``` Signed-off-by: Dan Rose <dan@digilabs.io> * clearer non-null check Signed-off-by: Dan Rose <dan@digilabs.io>
…dds#408) Also, allow multiple sanitizers. Signed-off-by: Dan Rose <dan@digilabs.io>
[test_subscriber-12] /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/ddsrt/src/mh3.c:28:53: runtime error: applying zero offset to null pointer [test_subscriber-12] SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/ros/master/src/eclipse-cyclonedds/cyclonedds/src/ddsrt/src/mh3.c:28:53 in Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
* Fix issue in dds_create_topic_arbitrary Changed the behaviour of dds_create_topic_arbitrary with respect to the sertopic parameter: the existing function dds_create_topic_arbitrary is marked deprecated and replaced by dds_create_topic_generic, which returns the sertopic that is actually used in as an out parameter. This can be eiter the provided sertopic (if this sertopic was not yet known in the domain) or an existing sertopic if the sertopic was registered earlier. Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com> * Fix memory leaks in case topic creation fails. Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>
* IPv6 extensions to patterns * use full GUID prefix for Cyclone * pattern fixes to deal with small changes in the formatting of QoS * suppressinof local built-in topic publications * asymmetrical disconnect detection improvements (better chance of detecting it, plus better suppression of spurious notifications) Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
This is a workaround for interoperability issues, ultimately driven by a Windows quirk that makes multicast delivery within a machine utterly unreliable if the transmitting socket is bound to 0.0.0.0 (despite all sockets having multicast interfaces set correctly) when there are also sockets transmitting to the same multicast group that have been bound to non-0.0.0.0. (Note: there may be other factors at play, but this is what it looks like after experimentation.) At least Fast-RTPS in some versions binds the socket it uses for transmitting multicasts to non-0.0.0.0, so interoperability with Fast-RTPS on Windows requires us to bind the socket we use for transmitting multicasts (which was the same as the one we use for receiving unicast data) also to non-0.0.0.0 or our multicasts get dropped often. This would work fine if other implementations honoured the set of advertised addresses. However, at least Fast-RTPS and Connext (in some versions) fail to do this and happily substitute 127.0.0.1 for the advertised IP address. If we bind to, e.g., 192.168.1.1, then suddenly those packets won't arrive anymore, breaking interoperability. The only work around is to use a separate socket for sending. Signed-off-by: Erik Boasson <eb@ilities.com>
wsock32.lib is only needed for the legacy version of Winsock and is not needed with Winsock2 (the current version). This appears to be a root cause of the multicast issue on Win10 and may allow us to reverse eclipse-cyclonedds#404 Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Thijs Sassen <thijs.sassen@adlinktech.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
* Move wctime, mtime, etime types to ddsrt * Add ddsrt_time_wallclock * Change ddsrt_time_monontic, elapsed to use mtime, etime types * Remove now, now_mt, now_et * Rename X_to_sec_usec to ddsrt_X_to_sec_usec * add_duration_to_X to ddsrt_X_add_duration (to be in line with the existing ddsrt_time_add_duration) * elimination of ddsrt/timeconv.h, it added more in the way of complications than it did in making things more elegant * rename of q_time.[ch] to ddsi_time.[ch]: that now only deals with DDSI timestamps and durations on the wire Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
The participant listener creates a pong writer, setting a publication matched listener on it. That listener can be invoked immediately and as it queries the subscriptions reader, it must not be enabled before the latter reader has been created. Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
|
LGTM! |
|
Thanks @dennis-adlink — I much prefer to have other people doing (at least) sanity checks on my PRs. The ECA validation failure is triggered by a GitHub user name in an Author field, but the original commits were all fine and hence is not blocking. |
eboasson
force-pushed
the
master-to-security
branch
from
a825d6c
to
67c4923
Compare
|
@dennis-adlink I discovered just in time that while I had updated the PR to include the latest changes in master, I had forgotten to also update it to the latest version of security. Just to be sure everything fits together properly, I have updated it to be a merge of head-of-master to head-of-security as of this today (in a single merge commit, given that I had to do some rework anyway to avoid a mes[hs] of merges). I'm pretty sure it's fine, but if you would be willing to re-do the sanity check, I'd be much obliged. My apologies for having wasted (some of) your time ... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.