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

[castler]

Resolves the majority of RULE-7-0-5 findings in the triage database using a mix of 3 minimal, unavoidable MISRA deviations, integer-type widening where a narrow type is purely a local/free-standing counter, and score::safe_math based code fixes everywhere the narrow width is load-bearing (bit-packed state, ABI/atomic-width invariants, or external/template function signatures) -- avoiding an excessive number of deviation comments for cases that are ordinary runtime signedness changes and can be made safe in code.

Note: event_data_control_composite.cpp (2 findings, SlotIndexType loop comparisons) is intentionally left unfixed for now and will be handled in a follow-up; SlotIndexType is a shared, binding-wide type used to index EventDataControl::state_slots_ in Shared Memory, so it needs more careful design discussion before deciding between safe_math and any alternative.

Deviations (kept to only what truly cannot be expressed in code):

• switch-enum-underlying-type-discriminant: switch/case discriminant promotion is a fixed language mechanism and cannot be intercepted by any function (3 files, 15 findings).
• ctype-function-argument-promotion: functions mandate an int parameter per the standard; the promotion is required by the library API contract, not by this code (instance_specifier.cpp).
• compile-time-static-assert-limit-check: static_assert-only check, no runtime value is ever computed (tracing_runtime.cpp).

Widened to std::uint32_t (rank >= int, so no promotion or signedness change ever occurs, per RULE-7-0-5's own semantics) instead of using safe_math, since these are pure local/free-standing counters with no persisted, shared-memory, or external-ABI role:

• memory_region_map.cpp/.h (both AcquireLatestRegionVersionForRead's and AcquireRegionVersionForOverwrite's outer retry-count loops -- each is an independent, arbitrary retry counter unrelated to VERSION_COUNT; plus VERSION_COUNT itself and AcquireRegionVersionForOverwrite's inner loop_idx/VERSION_COUNT loop, once VERSION_COUNT's type was widened from std::uint8_t to std::uint32_t. This is safe because VERSION_COUNT is only used as a std::array size and in this loop's own bound/modulo -- its value stays 10 and the <= 255 static_assert still holds; the actual load-bearing 8-bit invariant lives in latest_known_region_version_ (an atomic_uint8_t, unchanged) and version_idx, which is still explicitly cast back to std::uint8_t before being used/returned. Removing the loop_idx/VERSION_COUNT promotion this way also let the loop_idx + current_version addition drop safe_math::Add's overflow check entirely: with loop_idx in [1, 9] and current_version in [0, 255], the uint32_t sum can never overflow)
• transaction_log_local_view.cpp (retry/kRetryCount)
• flag_file_crawler.cpp/.h (max_number_of_retries/current_retry_count)
• tracing_runtime.cpp (impl) + .h (debounce_counter_/kDebounceAfter)
• proxy.cpp (retry_counter/max_retries/kMaxFlockRetries)
• message_passing_service_instance.cpp/.h (kMaxReceiveHandlersPerEvent loop and its consumer loop only; the sibling CopyNodeIdentifiers loop keeps safe_math since its uint8_t return type is a shared external template function signature)

Fixed a latent truncation bug while resolving RULE-7-0-5, rather than widening around it:

• subscription_subscription_pending_states.cpp (SubscribeEvent): the pre-existing code truncated the incoming std::size_t max_sample_count down to std::uint8_t before comparing it against the stored max_sample_count_ (std::uint16_t, legitimately up to 65535). That truncation silently corrupted the comparison for any max_sample_count above 255, which would have made a legitimate resubscription with the same, larger count spuriously fail with kMaxSampleCountNotRealizable. Fixed by comparing max_sample_count directly against max_sample_count_ widened to std::size_t (always lossless, and avoids the promotion since std::size_t already has rank >= int), instead of truncating one side down to std::uint8_t and then widening the truncated value back up to std::uint32_t for the comparison, which fixed the promotion but preserved the underlying data-loss bug.

Widened only a local, comparison-only copy (the canonical/stored value keeps its narrow, wire-format-tied type):

• unix_domain_engine.cpp (ReceiveProtocolMessage): the wire field size must stay std::uint16_t -- it is read directly into raw memory via an iovec (io[1].iov_len = sizeof(size)), so its exact 2-byte width is the protocol written by SendProtocolMessage. std::size_t is not a safe substitute either, since its width is platform/bitness-dependent and would desync 32-bit/64-bit peers. Instead, a local const std::size_t message_size = size; copy (safe: size's full 0..65535 range is always representable) is used for the two same-signedness comparisons that safe_math previously guarded. The one genuinely cross-signed comparison left (recvmsg's ssize_t byte count vs. the received size) is resolved with an explicit, justified static_caststd::size_t instead of safe_math, since has_value() already guarantees the count is non-negative.

Empirically verified via scoped CodeQL databases (per-package, RULE-7-0-5 query only) that none of the widened production sites produce findings.

All other findings -- same-type and cross-type comparisons/arithmetic alike -- are fixed in code using score::safe_math's CmpEqual/CmpNotEqual/ CmpLess/CmpGreater/CmpLessEqual/CmpGreaterEqual comparison helpers and Add/SubtractReturnMode::kAbortOnError arithmetic helpers (bounds-safe, enforced at the call site) across the remaining files, including:

• shared_memory_resource.cpp (uid comparison)
• event_subscription_control.cpp (bit-packed slot/subscriber-count state)
• element_fq_id.cpp, proxy_instance_identifier.cpp, skeleton_instance_identifier.cpp, lola_service_instance_identifier.cpp, lola_service_instance_id.cpp, unique_method_identifier.cpp (equality/ordering operators)
• binding_service_type_deployment_impl.h, lola_event_instance_deployment.cpp, trace_point_key.cpp, service_element_tracing_data.h, tracing_runtime.cpp (lola bindings), transaction_log_set.cpp, sample_allocatee_ptr.h, provider_event_data_control_local_view.cpp, subscription_subscribed_states.cpp

Non-integral sub-expressions (enum types, std::optional, map/struct comparisons) that safe_math cannot express are intentionally left as plain operators, since no signedness-changing promotion occurs there.

[crimson11]

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]

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

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]

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]

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]

I raised eclipse-score/baselibs#527 in baselibs

[limdor]

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

[crimson11]

According to your comment/doc:
Shouldn't we change i and num_ids_copied.first (1st member of the pair) to an uint32_t? This would/should remove the safe_math stuff!?

[crimson11]

Here I would clearly prefer to switch ServiceElementTracingData::SamplePointerIndex to being an uint32_tinstead of LolaEventInstanceDeployment::SampleSlotCountType (std::uint16_t) and document in code, why we chose a std::uint32_t although the valid reange would be just the range of a std::uint16_t ..

[crimson11]

Imho here it makes more sense to define slot_index as std::size_t(or decltype(state_slots_::size_type) and then we don't need safe_math

[crimson11]

would it be "low hanging fuits" to change event_slot_index_ already to an std::uint32_t and also kUninitialisedEventSlotIndex? Would have no impact outside I guess.