Os::File::open Bounded Inputs by LeStarch · Pull Request #4980 · nasa/fprime

LeStarch · 2026-04-09T17:58:11Z


*Related Issue(s)*
*Has Unit Tests (y/n)*
*Documentation Included (y/n)*
*Generative AI was used in this contribution (y/n)*

Change Description

Rationale

Testing/Review Recommendations

Future Work

AI Usage (see policy)

Add four new open() overloads to Os::File: - open(const char*, FwSizeType, Mode): bounded path without overwrite - open(const char*, FwSizeType, Mode, OverwriteType): bounded path with overwrite (core) - open(const Fw::StringBase&, Mode): StringBase without overwrite - open(const Fw::StringBase&, Mode, OverwriteType): StringBase with overwrite Refactor existing unbounded char* open() to delegate to the bounded version using FW_FIXED_LENGTH_STRING_SIZE as the default bound. The bounded char* with overwrite is now the core implementation; all other overloads delegate to it. The bounded core asserts that the path is null-terminated within the supplied length using Fw::StringUtils::string_length(). Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

FW_FIXED_LENGTH_STRING_SIZE is the max string length (256), not the buffer size. The length parameter represents buffer capacity including the null terminator, so pass STRING_SIZE + 1 (257) to match the Fw::String buffer size and allow full 256-character paths. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Os/File.cpp

+    return this->open(filepath, static_cast<FwSizeType>(FW_FIXED_LENGTH_STRING_SIZE + 1), requested_mode, overwrite);
+}
+
+File::Status File::open(const CHAR* filepath, FwSizeType length, File::Mode requested_mode) {


Os/File.cpp

+}
+
+File::Status File::open(const CHAR* filepath, FwSizeType length, File::Mode requested_mode) {
+    return this->open(filepath, length, requested_mode, OverwriteType::NO_OVERWRITE);


Os/File.cpp

+}
+
+File::Status File::open(const CHAR* filepath, FwSizeType length, File::Mode requested_mode) {
+    return this->open(filepath, length, requested_mode, OverwriteType::NO_OVERWRITE);


Os/File.cpp

+}
+
+File::Status File::open(const CHAR* filepath, FwSizeType length, File::Mode requested_mode) {
+    return this->open(filepath, length, requested_mode, OverwriteType::NO_OVERWRITE);


Os/File.cpp

+}
+
+File::Status File::open(const Fw::StringBase& path, File::Mode requested_mode, File::OverwriteType overwrite) {
+    return this->open(path.toChar(), static_cast<FwSizeType>(path.getCapacity()), requested_mode, overwrite);


Os/File.cpp

+}
+
+File::Status File::open(const Fw::StringBase& path, File::Mode requested_mode, File::OverwriteType overwrite) {
+    return this->open(path.toChar(), static_cast<FwSizeType>(path.getCapacity()), requested_mode, overwrite);


Os/File.cpp

+}
+
+File::Status File::open(const Fw::StringBase& path, File::Mode requested_mode, File::OverwriteType overwrite) {
+    return this->open(path.toChar(), static_cast<FwSizeType>(path.getCapacity()), requested_mode, overwrite);


LeStarch

Some things to fix.

Os/File.cpp

- Change open(const Fw::StringBase&, ...) to open(const Fw::ConstStringBase&, ...) to accept string literals directly - Replace FW_FIXED_LENGTH_STRING_SIZE + 1 with FileNameStringSize from FPP config for the default path length bound in unbounded open() - Include config/FppConstantsAc.hpp for FileNameStringSize constant - Run fprime-util format for code style compliance Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Add OpenFileCreateBounded and OpenFileCreateString rules to test the new Os::File::open() overloads: - OpenFileCreateBounded: tests open(const char*, FwSizeType, Mode, OverwriteType) - OpenFileCreateString: tests open(const Fw::ConstStringBase&, Mode, OverwriteType) Both rules are added to RandomizedInterfaceTesting and RandomizedTesting scenarios, plus dedicated Functionality tests. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Os/File.cpp

+    return this->open(filepath, length, requested_mode, OverwriteType::NO_OVERWRITE);
+}
+
+File::Status File::open(const CHAR* filepath,


Add OpenIllegalBoundedPath rule that verifies FW_ASSERT fires when a path buffer has no null terminator within the specified length bound. This exercises the string_length assertion in the bounded open() core. Added to both RandomizedInterfaceTesting and RandomizedTesting scenarios, plus a dedicated InvalidArguments.OpenBoundedPathUnterminated test. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Os/test/ut/file/CommonTests.cpp

Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Os/File.cpp

+    return this->open(filepath, length, requested_mode, OverwriteType::NO_OVERWRITE);
+}
+
+File::Status File::open(const CHAR* filepath,


Os/File.cpp


 File::Status File::open(const CHAR* filepath, File::Mode requested_mode, File::OverwriteType overwrite) {
+    FW_ASSERT(nullptr != filepath);
+    return this->open(filepath, static_cast<FwSizeType>(FileNameStringSize + 1), requested_mode, overwrite);


Os/File.cpp


 File::Status File::open(const CHAR* filepath, File::Mode requested_mode, File::OverwriteType overwrite) {
+    FW_ASSERT(nullptr != filepath);
+    return this->open(filepath, static_cast<FwSizeType>(FileNameStringSize + 1), requested_mode, overwrite);


Os/File.cpp

 }

+File::Status File::open(const Fw::ConstStringBase& path, File::Mode requested_mode) {
+    return this->open(path.toChar(), static_cast<FwSizeType>(path.getCapacity()), requested_mode,


Os/File.cpp

 }

+File::Status File::open(const Fw::ConstStringBase& path, File::Mode requested_mode) {
+    return this->open(path.toChar(), static_cast<FwSizeType>(path.getCapacity()), requested_mode,


The ConstStringBase open overload stores m_path pointing to the local Fw::String buffer, which is destroyed when the action returns. Reset m_path to point to the persistent filename string from the FILES vector after a successful open to prevent use-after-free in subsequent rules that call assert_file_consistent(). Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Use '!this->m_overwrite' guard in the duplicate-filename loop for OpenFileCreateBounded and OpenFileCreateString, matching the existing OpenBaseRule pattern. Without this guard, the loop can exhaust the filename pool (MAX_FILES=100) and fail with 'Failed to generate unique filename' when the FILES vector is full. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

…tion The filename pool (MAX_FILES=100) can be exhausted when multiple OPEN_CREATE rules with NO_OVERWRITE compete for unique filenames in randomized scenarios. Remove the uniqueness loop from OpenFileCreateBounded and OpenFileCreateString — if the file already exists, both open() and shadow_open() return the same error status, so the test still validates correctly without requiring unique filenames. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

The shadow file (SyntheticFile) uses a fresh filesystem per test while the real file (PosixFile) operates on the persistent disk filesystem. Without the uniqueness loop, filenames from prior tests that still exist on disk cause a mismatch: open() returns FILE_EXISTS but shadow_open() returns OP_OK, failing the ASSERT_EQ check. Restore the same uniqueness loop used by OpenBaseRule::action() in both OpenFileCreateBounded and OpenFileCreateString to ensure filenames are unique on the real filesystem before attempting to create. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

The uniqueness loop can exhaust the filename pool (get_filename caps at 101 entries) when many OPEN_CREATE rules compete in randomized scenarios. Once all cached filenames exist on disk, the loop spins forever. Replace with a simple early-return: if the randomly selected file already exists on the real filesystem, skip this iteration. The synthetic filesystem is reset per test and would not know about the file, causing a status mismatch. Skipping avoids both the mismatch and the exhaustion. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

…rules Increase MAX_FILES from 100 to 500 in PosixFileTests.cpp to give the filename pool more headroom. The pool caps at MAX_FILES+1 entries because get_filename picks in [0, MAX_FILES]; once FILES.size() exceeds that, only cached filenames are returned and new generation stops. Also change the uniqueness loop in OpenBaseRule, OpenFileCreateBounded, and OpenFileCreateString to gracefully skip the iteration (return) instead of failing with ASSERT_LT when a unique filename cannot be found. This prevents test explosions if the pool is ever exhausted again. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

The unbounded open(char*, Mode, OverwriteType) was hardcoding FileNameStringSize + 1 (201) as the default length bound, which tripped the string_length assert for any path longer than 200 characters. Fix by measuring the actual string length to compute the bound, restoring the pre-refactoring behavior where this overload accepts any valid null-terminated C string. Also factor the string_length call into a const and pass both the computed string length and the caller-supplied length as arguments to FW_ASSERT for improved diagnostics on assert failures. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Revert the unbounded open(char*, Mode, OverwriteType) back to using the correct F Prime bound of FileNameStringSize + 1 (201). Instead, cap the random test filename generation in PosixFileTests.cpp so the full path (BASE_PATH + '/' + random) fits within FileNameStringSize (200 chars). Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Os/File.cpp

+                        File::OverwriteType overwrite) {
    FW_ASSERT(&this->m_delegate == reinterpret_cast<FileInterface*>(&this->m_handle_storage[0]));
    FW_ASSERT(nullptr != filepath);
+    const FwSizeType string_len = static_cast<FwSizeType>(Fw::StringUtils::string_length(filepath, length));


Reduce MAX_RANDOM_LEN by 1 so full paths are strictly less than FileNameStringSize (max 199 chars instead of 200). This ensures the string_length check in the bounded open core has margin even at the boundary. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

Replace _POSIX_PATH_MAX with FileNameStringSize + 1 for full_buffer size and snprintf bound. The file path limit in F Prime is FileNameStringSize (200), not _POSIX_PATH_MAX (256). Using sizeof(full_buffer) in snprintf ensures the bound stays in sync with the buffer declaration. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

The test at line 255 passed a 201-char filename directly to Os::FileSystem::getFileSize(), which internally calls Os::File::open(). With the new bounded open implementation, this asserts because the path exceeds FileNameStringSize (200). The check was redundant since the test already verifies set_log_file() returned false and m_openFile is false. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>

devin-ai-integration bot and others added 2 commits April 9, 2026 16:55