refactor: api

[hsiuhsiu]

No description provided.

[cb-heimdall]

🟡 Heimdall Review Status

Requirement	Status	More Info
Reviews	🟡 0/2	Denominator calculation Show calculation 1 if user is bot 0 1 if user is external 0 2 if repo is sensitive 0 From .codeflow.yml 2 Additional review requirements Show calculation Max 0 0 From CODEOWNERS 1 Global minimum 0 Max 2 2 1 if commit is unverified 0 Sum 2
CODEOWNERS	✅ None for this change

In general, to fix “comparison result is always the same” issues, remove or restructure comparisons whose truth value is fully determined by earlier constraints, so that each conditional actually depends on runtime data. Where the comparison encodes a real safety concern (like avoiding overflow), ensure that the safety is handled in a way that is actually reachable, or enforce it via compile‑time checks instead.

Here, value_size_u32 is derived from header, and prior checks ensure value_size_u32 <= MAX_SERIALIZED_BIGNUM_BYTES. Since MAX_SERIALIZED_BIGNUM_BYTES is much smaller than INT_MAX, the additional runtime condition value_size_u32 > static_cast<uint32_t>(INT_MAX) is redundant and will never be true. The safest, minimal‑behaviour‑change fix is to:

• Replace the runtime check value_size_u32 > static_cast<uint32_t>(INT_MAX) with a compile‑time assertion that MAX_SERIALIZED_BIGNUM_BYTES <= INT_MAX. This preserves the original intent (never allow a value larger than INT_MAX to be cast to int) but avoids a dead comparison at runtime.
• Simplify the if condition to only check value_size_u32 > MAX_SERIALIZED_BIGNUM_BYTES, which is the actual bound that can be violated at runtime.

Concretely, inside bn_t::convert in src/cbmpc/crypto/base_bn.cpp, right after the existing static_assert at lines 620–621, add a second static_assert enforcing MAX_SERIALIZED_BIGNUM_BYTES <= INT_MAX. Then replace the compound if condition on line 641 with a single comparison against MAX_SERIALIZED_BIGNUM_BYTES. No new headers are required because <climits> or equivalent is presumably already available in the larger project; if not, that would be handled elsewhere since we cannot see the surrounding includes here.

In general, this problem is fixed by preventing addresses of stack-allocated objects from being stored in state that outlives the stack frame. Either (a) arrange that only pointers to objects with static/storage-duration or heap allocation are ever stored (and enforce that at the API boundary), or (b) change the storage so that it does not outlive the scope of the local object (e.g., by passing the pointer down the call chain instead of storing it globally or thread-locally).

For this specific case, the most straightforward fix without changing visible functionality is to restrict thread_local_storage_set_mod so that it never stores a pointer to a non-static object. We can use std::is_constant_evaluated() as a heuristic, but that doesn’t help with runtime objects. A better approach inside this file is to require that callers pass pointers to const mod_t objects with static storage duration. We cannot change all callers, but we can enforce safety right here by copying the value of mod_t into thread-local storage instead of storing the pointer directly. That way, any pointer passed in (whether to stack, global, or heap) is used only transiently for copying; the thread-local state holds its own independent mod_t with thread-local lifetime, eliminating dangling-pointer issues. Concretely:

• Replace static thread_local const mod_t* g_thread_local_storage_modo with static thread_local mod_t g_thread_local_storage_modo; (a by-value object).
• Change thread_local_storage_mod() to return a pointer to the thread-local object (&g_thread_local_storage_modo).
• Change thread_local_storage_set_mod(const mod_t* ptr) to copy *ptr into g_thread_local_storage_modo when ptr is non-null, and (optionally) reset it to a default-constructed mod_t when ptr is null.

These edits are all in src/cbmpc/crypto/base_bn.cpp, at lines 7–11 and 21. This assumes mod_t is copy-assignable, which is the minimally invasive assumption consistent with existing usage (the original code already assumed the pointed-to object would remain valid, which is stronger than assuming it is copyable). No new imports are strictly required for this change.

In general, to fix “comparison result is always the same” you either (1) correct the logic so that the comparison is meaningful in the reachable range of values, or (2) remove or adjust the comparison so that it is no longer redundant. Here, in the one-byte length branch ((b & 0x80) == 0), len is assigned from a single byte (len = b;), so len is always in [0, 255]. If MAX_CONVERT_LEN is larger than or equal to 255, then len > MAX_CONVERT_LEN can never be true in this branch, and this specific check is redundant. To preserve functionality while fixing the warning, we can make the check explicitly reflect the reachable range by also constraining the comparison to cases where len exceeds both the representable byte range and the configured maximum, e.g. if (len > 0xff && len > MAX_CONVERT_LEN), or more simply by removing this redundant check in the single-byte branch and relying on the multi-byte branches for larger lengths.

The safest and simplest fix, without changing overall behaviour, is to keep the structure of the code but eliminate the logically unreachable portion of the condition in this branch. Since len cannot exceed 0xff here, the “length too large” condition is effectively “never true” if MAX_CONVERT_LEN >= 0xff. If the intent is to permit smaller maximum lengths (e.g. MAX_CONVERT_LEN < 0xff), then the comparison is not redundant and CodeQL’s message does not apply. Because we are constrained to the visible code and cannot alter the definition of MAX_CONVERT_LEN, a minimal, semantics-preserving change that resolves the warning is to remove the MAX_CONVERT_LEN comparison in this specific branch, leaving the rest of the error handling intact. That is, we delete the if (len > MAX_CONVERT_LEN) { ... } block in the (b & 0x80) == 0 case, while leaving equivalent checks in the multi-byte branches unchanged.

Concretely, this means editing src/cbmpc/core/convert.cpp around lines 170–176. We will keep the assignment len = b; and the subsequent return; but remove the surrounding if (len > MAX_CONVERT_LEN) block, since the guard can never fire in this single-byte decoding branch when MAX_CONVERT_LEN is at least as large as the maximum encodable value in this format. No new methods, imports, or definitions are needed.

In general, an unused static function should either be removed or used. Removing it is safest when there is no real need for it; alternatively, if you want to keep the helper for readability, you can replace duplicated logic in the tests with calls to this function.

The minimal fix that does not alter existing test behavior is to remove the unused expect_eq definition entirely. The function simply wraps a sequence of GoogleTest assertions on two cmem_t buffers; if no existing tests call it, deleting it will not change program behavior. No other code in the shown snippet depends on it, and there are no associated declarations or forward declarations to adjust.

Specifically, in tests/unit/capi/test_pve_ac.cpp, remove lines 96–103 containing the definition of static void expect_eq(cmem_t a, cmem_t b) and its body, leaving the surrounding namespace and tests unchanged. No additional imports, methods, or definitions are needed.

In general, to fix an unused local variable introduced via structured bindings, remove the unused variable from the binding. In C++17 structured bindings, you cannot partially omit names inside the brackets, but you can avoid structured bindings altogether and bind only the value(s) you need (e.g., take the second member of the std::pair returned by emplace). This preserves all existing behavior while removing unused locals.

The single best way here is to replace const auto [it, inserted] = out.emplace(...); with a single-variable initialization that extracts only the second member of the returned std::pair, like:

const auto inserted = out.emplace(...).second;

This leaves the semantics unchanged, still checking inserted for duplicates, while eliminating the unused it. We should make this edit in both helper functions:

• In validate_leaf_key_mapping, lines 32–33.
• In validate_leaf_share_mapping, lines 52–53.

No new methods, imports, or definitions are needed; we are only changing the local-variable declaration style.

In general, unused local variables should be removed or, if part of a structured binding, replaced with a placeholder so that only the needed values are bound. This avoids warnings and clarifies that the unused value is intentionally ignored.

Here, the best fix is to keep using emplace and the inserted flag, but drop the unused iterator from the structured binding by using _ (or another throwaway name) instead of it. This keeps functionality identical: we still test inserted to detect duplicate leaf names and return E_BADARG in that case. The change should be applied at line 52 in src/cbmpc/capi/pve_ac.cpp within validate_leaf_share_mapping. No new methods or imports are needed; it’s purely a local variable name change in the binding.