Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
56 changes: 45 additions & 11 deletions Fw/Types/Assert.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,37 @@
// Return all the arguments of the macro, but the first one
#define FW_ASSERT_NO_FIRST_ARG(ARG_0, ...) __VA_ARGS__

// Define FW_UNREACHABLE to hint to the compiler that a code path is unreachable.
// Falls back to a no-op on compilers that do not support __builtin_unreachable.
//
// Two-tier detection:
// 1. __has_builtin — the modern check, available in Clang and GCC >= 10.
// 2. defined(__GNUC__) — catches older GCC (4.5–9) which support the builtin
// but lack __has_builtin. The #ifndef guard ensures only one definition wins.
#ifdef __has_builtin
#if __has_builtin(__builtin_unreachable)
#define FW_UNREACHABLE() __builtin_unreachable()
#endif
#endif

#ifndef FW_UNREACHABLE
#if defined(__GNUC__)
#define FW_UNREACHABLE() __builtin_unreachable()
#else
#define FW_UNREACHABLE() ((void)0) // no-op fallback for unknown compilers
#endif
#endif

// FW_ASSERT_UNREACHABLE is the hint used inside FW_ASSERT macros.
// Only active when FW_ASSERTIONS_ALWAYS_ABORT is enabled, because SwAssert
// can legally return when it is disabled (the default). Using __builtin_unreachable
// after a call that returns is undefined behavior.
#if FW_ASSERTIONS_ALWAYS_ABORT
#define FW_ASSERT_UNREACHABLE() FW_UNREACHABLE()
#else
#define FW_ASSERT_UNREACHABLE() ((void)0)
#endif

#if FW_ASSERT_LEVEL == FW_NO_ASSERT
// Users may override the NO_ASSERT case should they choose
#ifndef FW_ASSERT
Expand All @@ -23,24 +54,27 @@
#define FW_ASSERT(...) \
((FW_ASSERT_FIRST_ARG(__VA_ARGS__, 0)) \
? ((void)0) \
: (Fw::SwAssert(ASSERT_FILE_ID, FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__))))
: (Fw::SwAssert(ASSERT_FILE_ID, FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__)), FW_ASSERT_UNREACHABLE()))
#elif FW_ASSERT_LEVEL == FW_FILEID_ASSERT && !defined ASSERT_FILE_ID
#define FILE_NAME_ARG U32
#define FW_ASSERT(...) \
((FW_ASSERT_FIRST_ARG(__VA_ARGS__, 0)) \
? ((void)0) \
: (Fw::SwAssert(static_cast<U32>(0), FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__))))
#define FW_ASSERT(...) \
((FW_ASSERT_FIRST_ARG(__VA_ARGS__, 0)) \
? ((void)0) \
: (Fw::SwAssert(static_cast<U32>(0), FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__)), \
FW_ASSERT_UNREACHABLE()))
#elif FW_ASSERT_LEVEL == FW_RELATIVE_PATH_ASSERT && defined ASSERT_RELATIVE_PATH
#define FILE_NAME_ARG const CHAR*
#define FW_ASSERT(...) \
((FW_ASSERT_FIRST_ARG(__VA_ARGS__, 0)) \
? ((void)0) \
: (Fw::SwAssert(ASSERT_RELATIVE_PATH, FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__)), \
FW_ASSERT_UNREACHABLE()))
#else
#define FILE_NAME_ARG const CHAR*
#define FW_ASSERT(...) \
((FW_ASSERT_FIRST_ARG(__VA_ARGS__, 0)) \
? ((void)0) \
: (Fw::SwAssert(ASSERT_RELATIVE_PATH, FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__))))
#else
#define FILE_NAME_ARG const CHAR*
#define FW_ASSERT(...) \
((FW_ASSERT_FIRST_ARG(__VA_ARGS__, 0)) ? ((void)0) \
: (Fw::SwAssert(__FILE__, FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__))))
: (Fw::SwAssert(__FILE__, FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__)), FW_ASSERT_UNREACHABLE()))
#endif
#endif // if ASSERT is defined

Expand Down
58 changes: 47 additions & 11 deletions Svc/FileWorker/FileWorker.cpp

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🚩 writeIn_handler reports FW_STATUS_DONE_WRITE even when writeBufferToFile fails

At Svc/FileWorker/FileWorker.cpp:182, writeDoneOut_out always reports FW_STATUS_DONE_WRITE regardless of whether writeBufferToFile returned false (indicating a write failure). This is pre-existing behavior unchanged by this PR, but it means callers cannot distinguish a successful write from a failed one via the done port status. The only indicator of failure is the WriteFileError event emitted inside writeToFile.

(Refers to line 182)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,17 @@ void FileWorker ::cancelIn_handler(FwIndexType portNum) {
}

void FileWorker ::readIn_handler(FwIndexType portNum, const Fw::StringBase& path, Fw::Buffer& buffer) {
FW_ASSERT(path != nullptr);
FW_ASSERT(path.length() > 0);
FW_ASSERT(buffer.getData() != nullptr);
// Validate inputs before processing file
if (path.length() == 0) {
this->log_WARNING_HI_InvalidInput(Fw::LogStringArg("readIn"), Fw::LogStringArg("empty path"));
this->readDoneOut_out(0, FW_STATUS_INVALID_INPUT, 0);
return;
}
if (!buffer.isValid()) {
this->log_WARNING_HI_InvalidInput(Fw::LogStringArg("readIn"), Fw::LogStringArg("invalid buffer"));
this->readDoneOut_out(0, FW_STATUS_INVALID_INPUT, 0);
return;
}

const char* const fileName = path.toChar();
FwSizeType fileSize = 0;
Expand Down Expand Up @@ -62,7 +70,13 @@ void FileWorker ::readIn_handler(FwIndexType portNum, const Fw::StringBase& path

// Get filesize
Os::FileSystem::Status fsStat = Os::FileSystem::getFileSize(fileName, fileSize);
FW_ASSERT(fsStat == Os::FileSystem::OP_OK, fsStat); // file size checked with checksum
if (fsStat != Os::FileSystem::OP_OK) {
// Path is ground-controlled and the file may change between the CRC check and here
this->log_WARNING_HI_ReadFailedFileSize(fsStat);
this->readDoneOut_out(0, FW_STATUS_FAILED_FILE_SIZE, 0);
this->m_state = FW_STATE_IDLE;
return;
}

// Start reading
FileWorkerStatus workerStat = this->readBufferFromFile(buffer, fileName);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🚩 Pre-existing: readBufferFromFile always returns FW_STATUS_DONE_READ even on read errors

The readBufferFromFile function at line 191-216 always returns FW_STATUS_DONE_READ as long as the file was opened successfully, regardless of whether readFile encountered a read error, abort, or timeout inside readFileBytes. The readIn_handler at line 85 then passes this status to readDoneOut_out. This means callers receive FW_STATUS_DONE_READ even when the read didn't fully complete. This is a pre-existing issue not introduced by this PR, but the PR's changes (adding validation and the getFileSize error path) make the error-handling picture more complete without addressing this gap.

(Refers to lines 82-86)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand All @@ -73,8 +87,12 @@ void FileWorker ::readIn_handler(FwIndexType portNum, const Fw::StringBase& path
}

void FileWorker ::verifyIn_handler(FwIndexType portNum, const Fw::StringBase& path, U32 crc) {
FW_ASSERT(path != nullptr);
FW_ASSERT(path.length() > 0);
// Validate inputs before processing file
if (path.length() == 0) {
this->log_WARNING_HI_InvalidInput(Fw::LogStringArg("verifyIn"), Fw::LogStringArg("empty path"));
this->verifyDoneOut_out(0, FW_STATUS_INVALID_INPUT, 0);
return;
}

const char* const fileName = path.toChar();
FwSizeType fileSize = 0;
Expand Down Expand Up @@ -109,10 +127,22 @@ void FileWorker ::writeIn_handler(FwIndexType portNum,
Fw::Buffer& buffer,
FwSizeType offsetBytes,
bool append) {
FW_ASSERT(path != nullptr);
FW_ASSERT(path.length() > 0);
FW_ASSERT(buffer.getData() != nullptr);
FW_ASSERT(offsetBytes <= buffer.getSize());
// Validate inputs before processing file
if (path.length() == 0) {
this->log_WARNING_HI_InvalidInput(Fw::LogStringArg("writeIn"), Fw::LogStringArg("empty path"));
this->writeDoneOut_out(0, FW_STATUS_INVALID_INPUT, 0);
return;
}
if (!buffer.isValid()) {
this->log_WARNING_HI_InvalidInput(Fw::LogStringArg("writeIn"), Fw::LogStringArg("invalid buffer"));
this->writeDoneOut_out(0, FW_STATUS_INVALID_INPUT, 0);
return;
}
if (offsetBytes > buffer.getSize()) {
this->log_WARNING_HI_InvalidInput(Fw::LogStringArg("writeIn"), Fw::LogStringArg("invalid offset"));
this->writeDoneOut_out(0, FW_STATUS_INVALID_INPUT, 0);
return;
Comment on lines +141 to +144

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🚩 Edge case: offsetBytes == buffer.getSize() passes validation but triggers downstream assert

In writeIn_handler, the validation at line 141 checks offsetBytes > buffer.getSize(). When offsetBytes == buffer.getSize(), it passes validation, but downstream in writeBufferToFile -> writeToFile, the computed size becomes 0, which triggers FW_ASSERT(size > 0) at Svc/FileWorker/FileWorker.cpp:466. This is a pre-existing edge case (the old FW_ASSERT(offsetBytes <= buffer.getSize()) had the same semantics), but now that the handler is doing explicit input validation, it might be worth also rejecting offsetBytes == buffer.getSize() or handling the zero-byte-write case.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}

char fileName[FileNameStringSize];

Expand All @@ -132,7 +162,13 @@ void FileWorker ::writeIn_handler(FwIndexType portNum,
// NB: may count null terminator due to FPRIME/fprime-sw#57, but should still be less than FileNameStringSize in any
// case
FwSizeType length = Fw::StringUtils::string_length(path.toChar(), FileNameStringSize);
FW_ASSERT(length < FileNameStringSize && length < sizeof(fileName));
if (length >= FileNameStringSize || length >= sizeof(fileName)) {
// Path length is ground-controlled, so an oversized path is invalid input, not a coding error.
this->log_WARNING_HI_InvalidInput(Fw::LogStringArg("writeIn"), Fw::LogStringArg("path too long"));
this->writeDoneOut_out(0, FW_STATUS_INVALID_INPUT, 0);
this->m_state = FW_STATE_IDLE;
return;
}

(void)Fw::StringUtils::string_copy(fileName, path.toChar(), sizeof(fileName));
fileName[sizeof(fileName) - 1] = 0; // guarantee termination
Expand Down
46 changes: 25 additions & 21 deletions Svc/FileWorker/FileWorker.fpp
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
module Svc {

active component FileWorker {

# ----------------------------------------------------------------------
# Ports
# ----------------------------------------------------------------------
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
#Ports
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
Comment on lines +3 to +5

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🚩 FPP comment style change is a significant formatting regression

The section header comments in FileWorker.fpp were changed from properly indented # ------ style to flush-left #-- -- -- style. While both are valid FPP comments, the new style breaks the visual indentation hierarchy within the active component block and uses a non-standard dash-space pattern (-- -- --) instead of the continuous dashes (------) used throughout other .fpp files in the repository. This appears to be an accidental reformatting, possibly by an automated tool.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


@ Request to write to a file
async input port writeIn: Svc.FileWrite
Expand All @@ -27,9 +25,9 @@ module Svc {
@ Cancels a current operation
guarded input port cancelIn: Svc.CancelStatus

# ----------------------------------------------------------------------
# Standard ports
# ----------------------------------------------------------------------
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
#Standard ports
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --

@ Port for requesting the current time
time get port timeCaller
Expand Down Expand Up @@ -58,21 +56,21 @@ module Svc {
@ Port to set the value of a parameter
param set port prmSetOut

# ----------------------------------------------------------------------
# Commands
# ----------------------------------------------------------------------
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
#Commands
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --

# ----------------------------------------------------------------------
# Telemetry
# ----------------------------------------------------------------------
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
#Telemetry
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --

# ----------------------------------------------------------------------
# Parameters
# ----------------------------------------------------------------------
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
#Parameters
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --

# ----------------------------------------------------------------------
# Events
# ----------------------------------------------------------------------
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
#Events
#-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --

@ Notify we are not in IDLE state
event NotInIdle(
Expand Down Expand Up @@ -220,6 +218,12 @@ module Svc {
fileName: string size FileNameStringSize
) severity warning low \
format "Aborted after {} of {} bytes written to {}"
}

@ Invalid input arguments received on a port handler
event InvalidInput(
handler: string size 16
issue: string size 32
) severity warning high \
format "Invalid input in {} handler: {}"
Comment on lines +222 to +227

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 New InvalidInput event lacks throttle, unlike all other externally-triggered warning high events

The new InvalidInput event is declared as severity warning high but has no throttle or every clause. Every other externally-triggered warning high event in this component (NotInIdle, CrcFailed, ReadFailedFileSize, OpenFileError, WriteFileError, WriteValidationOpenError, WriteValidationReadError) uses throttle 2 every {seconds=10}. Since InvalidInput fires before any state transition (i.e. the component remains in IDLE), a caller repeatedly sending malformed async requests can flood the event log without any rate limiting. This is the exact scenario the throttle mechanism was designed to prevent, and exactly why NotInIdle (which also fires pre-state-check) includes it.

Suggested change
@ Invalid input arguments received on a port handler
event InvalidInput(
handler: string size 16
issue: string size 32
) severity warning high \
format "Invalid input in {} handler: {}"
@ Invalid input arguments received on a port handler
event InvalidInput(
handler: string size 16
issue: string size 32
) severity warning high \
format "Invalid input in {} handler: {}" \
throttle 2 \
every {seconds=10}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}
}
3 changes: 2 additions & 1 deletion Svc/FileWorker/FileWorkerTypes.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ typedef enum {
FW_STATUS_DONE_WRITE,
FW_STATUS_START_READ,
FW_STATUS_DONE_READ,
FW_STATUS_DONE
FW_STATUS_DONE,
FW_STATUS_INVALID_INPUT
} FileWorkerStatus;

typedef enum { FW_READ_DONE = 0, FW_READ_ERROR, FW_READ_ABORT, FW_READ_TIMEOUT, FW_READ_UNKNOWN } FileWorkerReadStatus;
Expand Down
35 changes: 21 additions & 14 deletions Svc/FileWorker/docs/sdd.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Svc::FileWorker is an active F' component used for writing and reading files on
|FileWorker-002|FileWorker shall provide an interface to write contents to a file passed in from a client-provided buffer|Unit Test|
|FileWorker-003|FileWorker shall handle receiving read-done signals while in improper state|Unit Test|
|FileWorker-004|FileWorker shall handle cancel for reads|Unit Test|
|FileWorker-005|FileWorker shall validate the inputs to its read, write, and verify port handlers and report malformed inputs via an event and a failure status rather than asserting|Unit Test|

### Diagrams

Expand Down Expand Up @@ -63,36 +64,42 @@ bytesWritten</code></td><td><code>FwSizeType</code></td><td></td></tr><tr><td><c
<tr style = "border: 2px solid black;"><td rowspan="4"><code>WriteTimeout</code></td><td rowspan="4">warning high</td><td rowspan="4">"Failed after {} of {} bytes written to {} after exceeding timeout of {} microseconds"</td><td rowspan="4"></td><td><code>bytesWritten</code></td><td><code>FwSizeType</code></td><td></td></tr><tr><td><code>writeSize</code></td><td><code>FwSizeType</code></td><td></td></tr><tr><td><code>fileName</code></td><td><code>string</code></td><td></td></tr><tr><td><code>size FileNameStringSize
timeout</code></td><td><code>U64</code></td><td></td></tr>
<tr style = "border: 2px solid black;"><td rowspan="3"><code>WriteAborted</code></td><td rowspan="3">warning low</td><td rowspan="3">"Aborted after {} of {} bytes written to {}"</td><td rowspan="3"></td><td><code>bytesWritten</code></td><td><code>FwSizeType</code></td><td></td></tr><tr><td><code>writeSize</code></td><td><code>FwSizeType</code></td><td></td></tr><tr><td><code>fileName</code></td><td><code>string</code></td><td></td></tr>
<tr style = "border: 2px solid black;"><td rowspan="2"><code>InvalidInput</code></td><td rowspan="2">warning high</td><td rowspan="2">"Invalid input in {} handler: {}"</td><td rowspan="2"></td><td><code>handler</code></td><td><code>string</code></td><td></td></tr><tr><td><code>issue</code></td><td><code>string</code></td><td></td></tr>
</tbody></table>

### 3.4 Functional Description

Each `*In` port handler validates its arguments before performing any file operation. Malformed inputs — such as an empty path, an invalid (null or zero-length) buffer, a write offset past the end of the buffer, or a path too long for the filename buffer — are reported by emitting an `InvalidInput` warning event and returning `INVALID_INPUT` to the client, rather than triggering an `FW_ASSERT`. This ensures that invalid, externally supplied (e.g. ground-commanded) inputs are surfaced to operators as telemetry instead of causing an assertion failure.

#### 3.4.1 writeIn_handler

Calling component passes in a buffer for writing to file, the file location is *path*, and the offset to start writing at.

1. If not in IDLE state, return NOT_IDLE
2. Set state to WRITING
3. Open a file at the provided path and write contents of client-provided buffer to it
4. After file is finished being written to, also write a validation CRC file
5. Return to client with write size and set state to IDLE
1. Validate inputs. If the path is empty, the buffer is invalid, the write offset is past the end of the buffer, or the path is too long for the filename buffer, emit an `InvalidInput` event and return `INVALID_INPUT`
2. If not in IDLE state, return NOT_IDLE
3. Set state to WRITING
4. Open a file at the provided path and write contents of client-provided buffer to it
5. After file is finished being written to, also write a validation CRC file
6. Return to client with write size and set state to IDLE

#### 3.4.2 readIn_handler

Calling component passes in a file path *path* to read and a buffer for client to read from

1. If not in IDLE state, return NOT_IDLE
2. Set state to READING
3. Verify checksum. If checksum fails, return FAILED_CRC and set state to IDLE
4. Otherwise, get file size
5. Read from file into buffer provided by the client
6. Send back the buffer and set state to IDLE
1. Validate inputs. If the path is empty or the buffer is invalid, emit an `InvalidInput` event and return `INVALID_INPUT`
2. If not in IDLE state, return NOT_IDLE
3. Set state to READING
4. Verify checksum. If checksum fails, return FAILED_CRC and set state to IDLE
5. Otherwise, get file size. If getting the file size fails, emit `ReadFailedFileSize` and return `FAILED_FILE_SIZE`, setting state to IDLE
6. Read from file into buffer provided by the client
7. Send back the buffer and set state to IDLE

#### 3.4.3 verifyIn_handler

1. Verify checksum. If checksum fails, return FAILED_CRC
2. Get file size. If file size fails, return FAILED_FILE_SIZE
3. Return file size to client
1. Validate the input path. If the path is empty, emit an `InvalidInput` event and return `INVALID_INPUT`
2. Verify checksum. If checksum fails, return FAILED_CRC
3. Get file size. If file size fails, return FAILED_FILE_SIZE
4. Return file size to client

#### 3.4.4 cancelIn_handler

Expand Down
15 changes: 15 additions & 0 deletions Svc/FileWorker/test/ut/FileWorkerErrTestMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,21 @@ TEST(Nominal, testWriteHashErr) {
tester.testWriteHashErr();
}

TEST(Nominal, testReadInvalidInput) {
Svc::FileWorkerTester tester;
tester.testReadInvalidInput();
}

TEST(Nominal, testVerifyInvalidInput) {
Svc::FileWorkerTester tester;
tester.testVerifyInvalidInput();
}

TEST(Nominal, testWriteInvalidInput) {
Svc::FileWorkerTester tester;
tester.testWriteInvalidInput();
}

int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
Expand Down
Loading
Loading