Os static analysis findings

[Sarah5567]

Related Issue(s)	Os multiple static analysis findings
Has Unit Tests (y/n)	n
Documentation Included (y/n)	n
Generative AI was used in this contribution (y/n)	y

---

Change Description

This PR addresses high-severity static analysis findings reported by SonarQube and CodeSonar in the Os component:

• SonarQube rule S3657 (virtual assignment operators): The virtual assignment operators are defined as = delete and intentionally kept virtual to ensure the same deleted operator applies consistently in derived classes as well. No code change was made. a // NOSONAR (cpp:S3657) comment was added solely to suppress this warning.

• Uninitialized Variable (Os/File.cpp:272): Added explicit initialization to eliminate undefined behavior.

• Integer / Multiplication Overflow of Allocation Size: Added explicit overflow checks in Os/Generic/PriorityQueue.cpp and Os/Generic/Types/MaxHeap.cpp.

• Condition Variable Warnings:

  • Inappropriate Call Outside Loop in PosixConditionVariable::pend: No functional change. The API is intentionally designed to require the caller to handle spurious wakeups by calling pend() inside a loop.
  • Use of Condition Variable Signal in notify(): No functional change. The use of pthread_cond_signal is correct and intentional.

• File System Race Condition (Os/Posix/Directory.cpp): there is a time window between the call to mkdir(path, ...) and the later call to opendir(path). Since the filesystem is shared state, another process/thread can modify the directory entry at path in between these two operations. As a result, opendir may end up opening a directory that is not the one this function just created, or it may fail because the directory was removed/changed after creation.
  Is this actually something we need to prevent, or is the current behavior acceptable by design? Would be happy to hear feedback on this.

• Division By Zero (Os/Posix/FileSystem.cpp:86): Code already contained an explicit guard against block_size == 0.

AI Usage (see policy)

ChatGPT was used to help locate and understand the reported static analysis findings.

[thomas-bc]

@Sarah5567 thanks a lot for contributing another round of static analysis fixes.

Just a heads up, we're pretty busy this week and on top of that our CI system is failing because of a registry that was taken offline (was providing us with raspberrypi cross compilers). So we'll likely have to wait next week before we can start merging things again, unless we decide to overrule CI in the meantime.

[djbyrne17]

Hi, Thank you for this (and other PRs).
Please change the occurrences of NOSONAR to NO_CODESONAR. Thank you for noting the finding number for each; please also include a few words on why each finding is a false positive.

[Sarah5567]

Hi, thanks for the review. Below is the rationale behind each false positive.

cpp:S3657 - Remove the virtual specifier; polymorphism should not be used with assignment operators:
This warning is triggered on deleted assignment operators, e.g.: virtual QueueInterface& operator=(const QueueInterface& other) = delete;
The operator is intentionally deleted to forbid assignment. Keeping it virtual allows derived classes to explicitly override and delete the same assignment operator: QueueInterface& operator=(const QueueInterface& other) override = delete;

Condition Variable - Inappropriate Call Outside Loop
The finding refers to: pthread_cond_wait(...) inside PosixConditionVariable::pend.
This function is a thin wrapper around the POSIX API. The responsibility to call it inside a loop lies with the caller, which is the intended design of this abstraction.

Condition Variable - Use of Condition Variable Signal:
Triggered by: pthread_cond_signal(...) in PosixConditionVariable::notify.
This is a direct wrapper over the POSIX signaling primitive. Correct usage patterns (predicate protection, looping on wait, etc.) are handled at a higher level. The warning does not indicate an actual issue in this context.

File System Race Condition - Os/Posix/Directory.cpp:
This one may not be a strict false positive.
There is a potential TOCTOU window between mkdir() and opendir(), as the filesystem is shared state.
It is unclear whether this race needs mitigation in this layer or whether the current behavior is acceptable by design. I did not suppress this finding, and would be happy to hear feedback on this.

Division By Zero - Os/Posix/FileSystem.cpp:86:
The code explicitly guards against division by zero: if (block_size == 0) return OTHER_ERROR;
All subsequent divisions use block_size only after this check, so a division by zero cannot occur.

Please let me know if anything is unclear or if any of my assumptions are incorrect.

[Sarah5567]

Hi @djbyrne17, is there anything else I need to do regarding the CI? The formatting checks are failing because of NO_CODESONAR.
Also, please let me know if any other changes are required here.
Thank you

[djbyrne17]

Hi @Sarah5567, please run this command on each file that you changed:

fprime-util format -f <filename>

or if you're confident, this should get all files in a one-liner:

git diff --name-only --relative devel...HEAD | fprime-util format --stdin

Looks like the files were flagged because of using a single space before the comment, rather than a double space. Nit-picky computer; such is the price we pay for consistency! So the format cmd shown will modify your code to match the FPrime styles.

There's a note in our (Contributing Guidelines)[https://github.com/nasa/fprime/blob/devel/CONTRIBUTING.md] under "Helpful Hints", but since it's required we should move that to a different section; I'll get on that.

Thanks!

[djbyrne17]

Thank you! This looks very good. We're all running a 5-day Workshop this week, so may be slow with the final review and merge, but it's on track.