Skip to content

Fix RULE-7-0-5 (no-signedness-change-from-promotion) findings #1011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
castler wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix RULE-7-0-5 (no-signedness-change-from-promotion) findings #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions quality/static_analysis/coding-standards.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,21 @@ deviations:
code-identifier: "shared-memory-align-const-cast"
scope: "Applies only to the specific const_cast marked with this code identifier in score/memory/shared/shared_memory_resource.cpp. This code-identifier must not be used anywhere else in the codebase."
justification: "`do_allocation_algorithm` uses `std::align`, whose `void*&` out-parameter cannot bind to the `const void*` input pointer. `std::align` only reads and arithmetically adjusts the pointer value to compute an aligned address within the given buffer; it never writes through the pointer to modify the pointee (https://timsong-cpp.github.io/cppwp/n4659/ptr.align#lib:align). The const qualification of `alloc_start` is therefore not violated at runtime, and the `const_cast` is required purely to satisfy `std::align`'s non-const parameter type."
- rule-id: "RULE-7-0-5"
query-id: "cpp/misra/no-signedness-change-from-promotion"
code-identifier: "switch-enum-underlying-type-discriminant"
scope: "Applies only to the `switch` statement conditions and `case` labels marked with this code identifier in score/message_passing/client_connection.cpp, score/message_passing/unix_domain/unix_domain_server.cpp, and score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp. This code-identifier must not be used anywhere else in the codebase."
justification: "These `switch` statements dispatch on a message/protocol byte (an unsigned narrow integral type, e.g. `std::uint8_t`) against `case` labels built from `score::cpp::to_underlying(EnumType::Value)`. The C++ language unconditionally applies integral promotion to a `switch` statement's controlling expression, and separately converts every `case` label's constant expression to that same promoted type, before the dispatch comparison is performed. This conversion is a fixed part of the `switch`/`case` language mechanism itself: it cannot be intercepted, wrapped, or replaced by any function call (including safe-comparison helpers such as `score::safe_math::CmpEqual`), because a `case` label must be an integer constant expression usable directly in the statement, not a function call result. The dispatch is not a source of misbehavior: it is a straightforward point-wise comparison for equality between the promoted controlling value and each promoted case constant, which cannot be affected by the promotion's signedness change (both sides undergo the exact same conversion, so equality is preserved)."
- rule-id: "RULE-7-0-5"
query-id: "cpp/misra/no-signedness-change-from-promotion"
code-identifier: "ctype-function-argument-promotion"
scope: "Applies only to the argument-conversion of an `unsigned char` value passed to a `<cctype>` classification/conversion function (e.g. `std::isalpha`, `std::isalnum`), marked with this code identifier in score/mw/com/impl/instance_specifier.cpp. This code-identifier must not be used anywhere else in the codebase."
justification: "The C++ standard library's `<cctype>` functions take an `int` parameter that must hold either `EOF` or the value of an `unsigned char` (https://eel.is/c++draft/character.seq#classification.functions). Passing an `unsigned char` value to such a function unavoidably requires the compiler to implicitly convert it to `int`, which is exactly what the marked line does (`static_cast<unsigned char>(curent_char)` immediately followed by `std::isalpha`/`std::isalnum`). This conversion is mandated by the standard library's own API contract; it is not a signedness defect in this code, and there is no function-level wrapper that can avoid it, since the conversion happens at the call boundary of a fixed, non-overloadable standard library signature."
- rule-id: "RULE-7-0-5"
query-id: "cpp/misra/no-signedness-change-from-promotion"
code-identifier: "compile-time-static-assert-limit-check"
scope: "Applies only to the `static_assert` size-limit check marked with this code identifier in score/mw/com/impl/bindings/lola/tracing/tracing_runtime.cpp."
justification: "The marked expression appears solely inside a `static_assert` condition, evaluated entirely at compile time against `std::numeric_limits<...>::max()` constants; it never executes at runtime and can never observe or propagate a runtime value. Its only purpose is to fail the build if a future change to the underlying typedefs would allow the sum of the two maxima to exceed `TraceContextId`'s range. There is no runtime signedness-change defect for this rule to prevent, and using a `score::safe_math` runtime helper is neither necessary (the check is compile-time only) nor possible (safe_math's checked operations are not usable in this `static_assert`'s constant-expression context in a way that would improve on the existing explicit compile-time check)."
guideline-recategorizations:
- rule-id: "RULE-0-1-1"
category: "disapplied"
Expand Down
14 changes: 7 additions & 7 deletions score/memory/shared/memory_region_map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,8 @@ auto MemoryRegionMapImpl<AtomicIndirectorType>::AcquireLatestRegionVersionForRea
// It would actually need an insane number of threads, which concurrently try to increment the refcount!
// We are using this bounded number here instead of a while(true) construct as we are then able to return
// std::nullopt and leave it to the layer above to react e.g. with a std::terminate()!
constexpr std::uint8_t max_retries = 255U;
for (std::uint8_t retry_count = 0U; retry_count < max_retries; retry_count++)
constexpr std::uint32_t max_retries = 255U;
for (std::uint32_t retry_count = 0U; retry_count < max_retries; retry_count++)
{
const uint8_t region_index = latest_known_region_version_.load(std::memory_order_relaxed);
const std::uint32_t previous_refcount = AtomicIndirectorType<RegionVersionRefCountType>::fetch_add(
Expand Down Expand Up @@ -340,16 +340,16 @@ std::optional<std::uint8_t> MemoryRegionMapImpl<AtomicIndirectorType>::AcquireRe
{
// Arbitrary retry value here. It is expected, that when checking all known regions versions, the writer
// will find one being unused! Because readers are accessing only the latest for a very short time ...
constexpr std::uint8_t max_retries = 10U;
for (std::uint8_t retry_count = 0U; retry_count < max_retries; retry_count++)
constexpr std::uint32_t max_retries = 10U;
for (std::uint32_t retry_count = 0U; retry_count < max_retries; retry_count++)
{
// Iterate over version indices starting with the version directly after the current
// latest_known_region_version_. That way we are checking the oldest version first, to have the lowest
// probability of clashes with readers.
for (std::uint8_t loop_idx = 1U; loop_idx < VERSION_COUNT; loop_idx++)
for (std::uint32_t loop_idx = 1U; loop_idx < VERSION_COUNT; loop_idx++)
{
const auto version_idx = static_cast<std::uint8_t>(
(loop_idx + latest_known_region_version_.load(std::memory_order_relaxed)) % VERSION_COUNT);
const std::uint32_t current_version = latest_known_region_version_.load(std::memory_order_relaxed);
const auto version_idx = static_cast<std::uint8_t>((loop_idx + current_version) % VERSION_COUNT);
RegionVersionRefCountType cur_ref_count =
known_regions_versions_refcounts_.at(static_cast<size_t>(version_idx)).load();
if (cur_ref_count == 0U)
Expand Down
2 changes: 1 addition & 1 deletion score/memory/shared/memory_region_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class MemoryRegionMapImpl final
// being given values that are not subsequently used.".
// Rationale: False positive - variable is used below.
// coverity[autosar_cpp14_a0_1_1_violation : FALSE]
static constexpr const std::uint8_t VERSION_COUNT{10U};
static constexpr const std::uint32_t VERSION_COUNT{10U};
static_assert(
VERSION_COUNT <= 255U,
"VERSION_COUNT needs to be smaller than 255 as our latest_known_region_version_ tracker is an uint8 ");
Expand Down
2 changes: 1 addition & 1 deletion score/memory/shared/shared_memory_resource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ ShmObjectStatInfo GetShmObjectStatInfo(const ISharedMemoryResource::FileDescript
if (typed_memory_ptr != nullptr)
{
const auto typedmemd_uid = AcquireTypedMemoryDaemonUid();
if (is_named_shm && (typedmemd_uid.has_value() && (typedmemd_uid.value() == owner_uid)))
if (is_named_shm && (typedmemd_uid.has_value() && safe_math::CmpEqual(typedmemd_uid.value(), owner_uid)))

@crimson11 crimson11 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is safe_math::CmpEqual needed here?
Both operands boild down to being an uid_t! Which per the standard is an unsigned int?

But in this case neither a signed-integer promotion takes place ... and the safe_math::CmpEqual has no effect.
This is what the safe-math op does:

constexpr bool CmpEqual(Lhs lhs, Rhs rhs) noexcept
{
    using BiggerType = bigger_type_t<Lhs, Rhs>;
    return static_cast<BiggerType>(lhs) == static_cast<BiggerType>(rhs);
}

in this case BiggerType will stay uid_t (unsigned integer) and these casts have no effect! Wher am I wrong?

@limdor limdor Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which per the standard is an unsigned int?

I do not think so, I do not think you have any guarantees of size or signess, you only have guarantees of this being an integral type according to POSIX. Still if both are the same type, there could only be a promotion if they would be smaller than integer (int 16 or uin 16 for example)

But in this case, the finding claims that the type is unsigned int.

https://github.com/eclipse-score/communication/security/code-scanning/14104

Image

But then CodeQL is reporting that there is a promotion from unsigned int to long. I cannot see how this could be the case. This could only happen if the types being compared are different. If both are uid_t, I would say this is a CodeQL issue.

@LittleHuba LittleHuba Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No CodeQL is perfectly right on this.

The basic assumption of @crimson11 was just off.
owner_uid is typed as const auto. This means the variable will take as actual type the type of the variable that is assigned. This type is std::int64_t.
uid_t is an "arithmetic type of appropriate length" (https://pubs.opengroup.org/onlinepubs/007904875/basedefs/sys/types.h.html#tag_13_67).
Further in, there is the additional restriction "nlink_t, uid_t, gid_t, and id_t shall be integer types."

So POSIX does not enforce that uid_t is signed or unsigned. It may be either. On this system it seems to be an unsigned int.

This is a clear case, where using safemath is the right thing to do. It will correctly perform the comparison doing necessary casting on the fly.

@LittleHuba LittleHuba Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, baselibs has here a problem. They assume that uid_t is a signed integer, when it is perfectly allowed to be an unsigned integer. E.g. stat uses uid_t for st_uid while baselibs hardcodes this to a std::int64_t.

@LittleHuba LittleHuba Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raised eclipse-score/baselibs#527 in baselibs

@limdor limdor Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, baselibs has here a problem. They assume that uid_t is a signed integer, when it is perfectly allowed to be an unsigned integer. E.g. stat uses uid_t for st_uid while baselibs hardcodes this to a std::int64_t.

I was involved in some of the APIs of OSAL (I do not remember this one). For some the assumption was that the values are not bigger than int64 max even if it is unsigned. In the end in OSAL at least at the beginning the goal was to have fixed and os independent types. If this wants to be continued, then some assumption will have to be taken, we just need to make sure they are proper documented (potentially with also preconditions).

{
// Suppress "AUTOSAR C++14 A0-1-1", The rule states: "A project shall not contain instances of non-volatile
// variables being given values that are not subsequently used"
Expand Down
3 changes: 3 additions & 0 deletions score/message_passing/client_connection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,8 +468,10 @@ IClientConnection::StopReason ClientConnection::ProcessInputEvent() noexcept
// This switch statement is considered not well-formed due to early exits, i.e. return statements.
// New Misra rule 9.4.2 allows terminating switch statements with a return statement
// coverity[autosar_cpp14_m6_4_3_violation]
// Deviation of MISRA RULE-7-0-5: codeql::misra_deviation_next_line(switch-enum-underlying-type-discriminant)
switch (code)
{
// Deviation of MISRA RULE-7-0-5: codeql::misra_deviation_next_line(switch-enum-underlying-type-discriminant)
case score::cpp::to_underlying(ServerToClient::REPLY):
{
std::unique_lock<std::mutex> lock{send_mutex_};
Expand All @@ -487,6 +489,7 @@ IClientConnection::StopReason ClientConnection::ProcessInputEvent() noexcept
}
break;
}
// Deviation of MISRA RULE-7-0-5: codeql::misra_deviation_next_line(switch-enum-underlying-type-discriminant)
case score::cpp::to_underlying(ServerToClient::NOTIFY):
{
if (!notify_callback_.empty())
Expand Down
18 changes: 14 additions & 4 deletions score/message_passing/unix_domain/unix_domain_engine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,17 +247,23 @@ score::cpp::expected<score::cpp::span<const std::uint8_t>, score::os::Error> Uni
// other side disconnected
return score::cpp::make_unexpected(score::os::Error::createFromErrno(EPIPE));
}
if (size == 0)
// size itself must stay std::uint16_t: it is read directly into raw memory via the iovec above
// (io[1].iov_len = sizeof(size)), matching the exact 2-byte wire field written by SendProtocolMessage.
// Widening it (e.g. to std::size_t, whose width is even platform/bitness-dependent) would change the
// number of bytes read off the wire and break the protocol. Instead, widen only a local copy used for
// the comparisons below -- size's full range (0..65535) is always exactly representable in std::size_t.
const std::size_t message_size = size;
if (message_size == 0U)
{
return {};
}
if (size > static_cast<std::uint16_t>(posix_receive_buffer_.size()))
if (message_size > posix_receive_buffer_.size())
{
return score::cpp::make_unexpected(score::os::Error::createFromErrno(EMSGSIZE));
}

io[0].iov_base = posix_receive_buffer_.data();
io[0].iov_len = static_cast<std::size_t>(size);
io[0].iov_len = message_size;
msg.msg_iovlen = 1UL;

using MessageFlag = ::score::os::Socket::MessageFlag;
Expand All @@ -266,7 +272,11 @@ score::cpp::expected<score::cpp::span<const std::uint8_t>, score::os::Error> Uni
{
return score::cpp::make_unexpected(size_expected.error());
}
if (size_expected.value() != size)
// size_expected.value() is a recvmsg() byte count, which is guaranteed non-negative whenever has_value()
// is true (already checked above), so the signed-to-unsigned cast below never reinterprets a negative
// value.
// coverity[autosar_cpp14_a4_7_1_violation]
if (static_cast<std::size_t>(size_expected.value()) != message_size)
{
return score::cpp::make_unexpected(score::os::Error::createFromErrno(EIO));
}
Expand Down
3 changes: 3 additions & 0 deletions score/message_passing/unix_domain/unix_domain_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,17 @@ bool UnixDomainServer::ServerConnection::ProcessInput()
return false;
}
auto message = message_expected.value();
// Deviation of MISRA RULE-7-0-5: codeql::misra_deviation_next_line(switch-enum-underlying-type-discriminant)
switch (code)
{
// Deviation of MISRA RULE-7-0-5: codeql::misra_deviation_next_line(switch-enum-underlying-type-discriminant)
case score::cpp::to_underlying(ClientToServer::REQUEST):
return (std::holds_alternative<HandlerPointerT>(user_data)
? std::get<HandlerPointerT>(user_data)->OnMessageSentWithReply(*this, message)
: server_.sent_with_reply_callback_(*this, message))
.has_value();

// Deviation of MISRA RULE-7-0-5: codeql::misra_deviation_next_line(switch-enum-underlying-type-discriminant)
case score::cpp::to_underlying(ClientToServer::SEND):
return (std::holds_alternative<HandlerPointerT>(user_data)
? std::get<HandlerPointerT>(user_data)->OnMessageSent(*this, message)
Expand Down
8 changes: 8 additions & 0 deletions score/mw/com/impl/bindings/lola/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,7 @@ cc_library(
"//score/mw/com/impl:runtime",
"//score/mw/com/impl/bindings/lola:partial_restart_path_builder",
"@score_baselibs//score/language/safecpp/safe_atomics:try_atomic_add",
"@score_baselibs//score/language/safecpp/safe_math",
"@score_baselibs//score/os:errno_logging",
],
tags = ["FFI"],
Expand Down Expand Up @@ -491,6 +492,7 @@ cc_library(
deps = [
"//score/mw/com/impl:service_element_type",
"//score/mw/com/impl/configuration",
"@score_baselibs//score/language/safecpp/safe_math",
"@score_baselibs//score/mw/log",
],
)
Expand Down Expand Up @@ -560,6 +562,7 @@ cc_library(
"//score/mw/com/impl:subscription_state",
"//score/mw/com/impl:subscription_state_change_handler",
"//score/mw/com/impl/bindings/lola/messaging:i_message_passing_service",
"@score_baselibs//score/language/safecpp/safe_math",
],
)

Expand Down Expand Up @@ -642,6 +645,7 @@ cc_library(
":event_data_control",
":event_slot_status",
"@score_baselibs//score/concurrency:atomic_indirector",
"@score_baselibs//score/language/safecpp/safe_math",
],
)

Expand Down Expand Up @@ -764,6 +768,7 @@ cc_library(
deps = [
":control_slot_types",
":event_data_control_composite",
"@score_baselibs//score/language/safecpp/safe_math",
],
)

Expand Down Expand Up @@ -872,6 +877,7 @@ cc_library(
"//score/mw/com/impl/configuration",
"//score/mw/com/impl/util:copyable_atomic",
"@score_baselibs//score/containers:dynamic_array",
"@score_baselibs//score/language/safecpp/safe_math",
"@score_baselibs//score/result",
"@score_baselibs//score/scope_exit",
],
Expand Down Expand Up @@ -933,6 +939,7 @@ cc_library(
],
deps = [
"//score/mw/com/impl/configuration",
"@score_baselibs//score/language/safecpp/safe_math",
"@score_baselibs//score/mw/log",
],
)
Expand All @@ -948,6 +955,7 @@ cc_library(
],
deps = [
"//score/mw/com/impl/configuration",
"@score_baselibs//score/language/safecpp/safe_math",
"@score_baselibs//score/mw/log",
],
)
Expand Down
18 changes: 10 additions & 8 deletions score/mw/com/impl/bindings/lola/element_fq_id.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
********************************************************************************/
#include "score/mw/com/impl/bindings/lola/element_fq_id.h"

#include "score/language/safecpp/safe_math/safe_math.h"
#include "score/mw/log/logging.h"

#include <cstdint>
Expand Down Expand Up @@ -40,7 +41,7 @@ ElementFqId::ElementFqId(const ServiceId service_id,
// range coverity[autosar_cpp14_a7_2_1_violation]
: ElementFqId(service_id, element_id, instance_id, static_cast<ServiceElementType>(element_type))
{
if (element_type > static_cast<std::uint8_t>(ServiceElementType::FIELD))
if (safe_math::CmpGreater(element_type, static_cast<std::uint8_t>(ServiceElementType::FIELD)))
{
score::mw::log::LogFatal("lola") << "ElementFqId::ElementFqId failed: Invalid ServiceElementType:"
<< element_type;
Expand Down Expand Up @@ -83,21 +84,22 @@ bool operator==(const ElementFqId& lhs, const ElementFqId& rhs) noexcept
// This a false-positive, all operands are parenthesized.
// A bug ticket has been created to track this: [Ticket-165315](broken_link_j/Ticket-165315)
// coverity[autosar_cpp14_a5_2_6_violation : FALSE]
return ((lhs.service_id_ == rhs.service_id_) && (lhs.element_id_ == rhs.element_id_) &&
(lhs.instance_id_ == rhs.instance_id_));
return (safe_math::CmpEqual(lhs.service_id_, rhs.service_id_) &&
safe_math::CmpEqual(lhs.element_id_, rhs.element_id_) &&
safe_math::CmpEqual(lhs.instance_id_, rhs.instance_id_));
}

bool operator<(const ElementFqId& lhs, const ElementFqId& rhs) noexcept
{
if (lhs.service_id_ == rhs.service_id_)
if (safe_math::CmpEqual(lhs.service_id_, rhs.service_id_))
{
if (lhs.instance_id_ == rhs.instance_id_)
if (safe_math::CmpEqual(lhs.instance_id_, rhs.instance_id_))
{
return lhs.element_id_ < rhs.element_id_;
return safe_math::CmpLess(lhs.element_id_, rhs.element_id_);
}
return lhs.instance_id_ < rhs.instance_id_;
return safe_math::CmpLess(lhs.instance_id_, rhs.instance_id_);
}
return lhs.service_id_ < rhs.service_id_;
return safe_math::CmpLess(lhs.service_id_, rhs.service_id_);
}

// Suppress "AUTOSAR C++14 A13-2-2" rule finding: "A binary arithmetic operator and a bitwise operator shall return
Expand Down
Loading
Loading