Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
3 changes: 2 additions & 1 deletion Fw/Dp/DpContainer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ Fw::SerializeStatus DpContainer::deserializeHeader() {
if (status == Fw::FW_SERIALIZE_OK) {
const FwSizeType requestedSize = sizeof this->m_userData;
FwSizeType receivedSize = requestedSize;
status = deserializer.deserializeTo(this->m_userData, receivedSize, Fw::Serialization::OMIT_LENGTH);
status = deserializer.deserializeTo(this->m_userData, sizeof(this->m_userData), receivedSize,
Fw::Serialization::OMIT_LENGTH);
if (receivedSize != requestedSize) {
status = Fw::FW_DESERIALIZE_SIZE_MISMATCH;
}
Expand Down
3 changes: 2 additions & 1 deletion Fw/Dp/test/util/DpContainerHeader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ struct DpContainerHeader {
// Deserialize the user data
DpContainerHeader::moveDeserToOffset(file, line, deserializer, DpContainer::Header::USER_DATA_OFFSET);
FwSizeType size = sizeof this->m_userData;
status = deserializer.deserializeTo(this->m_userData, size, Fw::Serialization::OMIT_LENGTH);
status = deserializer.deserializeTo(this->m_userData, sizeof(this->m_userData), size,
Fw::Serialization::OMIT_LENGTH);
DP_CONTAINER_HEADER_ASSERT_EQ(status, FW_SERIALIZE_OK);
DP_CONTAINER_HEADER_ASSERT_EQ(size, sizeof this->m_userData);
// Deserialize the data product state
Expand Down
2 changes: 1 addition & 1 deletion Fw/FilePacket/PathName.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ SerializeStatus FilePacket::PathName ::fromSerialBuffer(SerialBuffer& serialBuff
{
const U8* addrLeft = serialBuffer.getBuffAddrLeft();
U8 bytes[MAX_LENGTH];
const SerializeStatus status = serialBuffer.popBytes(bytes, this->m_length);
const SerializeStatus status = serialBuffer.popBytes(bytes, MAX_LENGTH, this->m_length);

if (status != FW_SERIALIZE_OK) {
return status;
Expand Down
5 changes: 3 additions & 2 deletions Fw/Log/AmpcsEvrLogPacket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ SerializeStatus AmpcsEvrLogPacket::deserializeFrom(SerialBufferBase& buffer) {
SerializeStatus stat;

len = AMPCS_EVR_TASK_NAME_LEN;
stat = buffer.deserializeTo(this->m_taskName, len, true);
stat = buffer.deserializeTo(this->m_taskName, AMPCS_EVR_TASK_NAME_LEN, len, Fw::Serialization::OMIT_LENGTH);
if (stat != FW_SERIALIZE_OK) {
return stat;
}
Expand All @@ -69,7 +69,8 @@ SerializeStatus AmpcsEvrLogPacket::deserializeFrom(SerialBufferBase& buffer) {
}

FwSizeType size = buffer.getDeserializeSizeLeft();
stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), size, true);
stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), this->m_logBuffer.getCapacity(), size,
Fw::Serialization::OMIT_LENGTH);
if (stat == FW_SERIALIZE_OK) {
// Shouldn't fail
stat = this->m_logBuffer.setBuffLen(size);
Expand Down
3 changes: 2 additions & 1 deletion Fw/Log/LogPacket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ SerializeStatus LogPacket::deserializeFrom(SerialBufferBase& buffer, Fw::Endiann

// remainder of buffer must be telemetry value
FwSizeType size = buffer.getDeserializeSizeLeft();
stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), size, Fw::Serialization::OMIT_LENGTH);
stat = buffer.deserializeTo(this->m_logBuffer.getBuffAddr(), this->m_logBuffer.getCapacity(), size,
Fw::Serialization::OMIT_LENGTH);
if (stat == FW_SERIALIZE_OK) {
// Shouldn't fail
stat = this->m_logBuffer.setBuffLen(size);
Expand Down
36 changes: 26 additions & 10 deletions Fw/Tlm/TlmPacket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,25 @@
return stat;
}

// telemetry buffer
stat = this->m_tlmBuffer.deserializeTo(buffer.getBuffAddr(), bufferSize, Fw::Serialization::OMIT_LENGTH);
if (stat != Fw::FW_SERIALIZE_OK) {
return stat;
// validate telemetry buffer's destination pointer and capacity
U8* const buffAddr = buffer.getBuffAddr();

Check warning

Code scanning / CodeQL

Unchecked function argument Warning

This use of parameter buffer has not been checked.
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
if (buffAddr == nullptr) {
return Fw::FW_DESERIALIZE_SIZE_MISMATCH;
}

// set buffer size
stat = buffer.setBuffLen(bufferSize);
if (bufferSize == 0 || bufferSize > buffer.getCapacity()) {
return Fw::FW_DESERIALIZE_SIZE_MISMATCH;
}
stat = this->m_tlmBuffer.deserializeTo(buffAddr, buffer.getCapacity(), bufferSize,
Fw::Serialization::OMIT_LENGTH);
if (stat != Fw::FW_SERIALIZE_OK) {
return stat;
}

return Fw::FW_SERIALIZE_OK;
// Update the destination buffer's length so callers can deserialize typed
// values out of it afterwards (e.g. buffOut.deserializeTo(valOut)).
// Without this, buffer.getBuffLength() stays 0 and any subsequent typed
// deserialize call returns FW_DESERIALIZE_SIZE_MISMATCH.
return buffer.setBuffLen(bufferSize);
}

SerializeStatus TlmPacket::serializeTo(SerialBufferBase& buffer, Fw::Endianness mode) const {
Expand All @@ -140,13 +146,23 @@
if (stat != Fw::FW_SERIALIZE_OK) {
return stat;
}

// deserialize the channel value entry buffers
FwSizeType size = buffer.getDeserializeSizeLeft();
stat = buffer.deserializeTo(this->m_tlmBuffer.getBuffAddr(), size, Fw::Serialization::OMIT_LENGTH);

// validate destination pointer and capacity
U8* const tlmAddr = this->m_tlmBuffer.getBuffAddr();
if (tlmAddr == nullptr) {
return Fw::FW_DESERIALIZE_SIZE_MISMATCH;
}
if (size > this->m_tlmBuffer.getCapacity()) {
return Fw::FW_DESERIALIZE_SIZE_MISMATCH;
}
stat = buffer.deserializeTo(tlmAddr, this->m_tlmBuffer.getCapacity(), size,
Fw::Serialization::OMIT_LENGTH);
if (stat == FW_SERIALIZE_OK) {
// Shouldn't fail
stat = this->m_tlmBuffer.setBuffLen(size);
FW_ASSERT(stat == FW_SERIALIZE_OK, static_cast<FwAssertArgType>(stat));
}
return stat;
}
Expand Down
14 changes: 12 additions & 2 deletions Fw/Types/SerialBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,18 @@ SerializeStatus SerialBuffer ::pushBytes(const U8* const addr, const FwSizeType
return this->serializeFrom(const_cast<U8*>(addr), n, Fw::Serialization::OMIT_LENGTH);
}

SerializeStatus SerialBuffer ::popBytes(U8* const addr, FwSizeType n) {
return this->deserializeTo(addr, n, Fw::Serialization::OMIT_LENGTH);
SerializeStatus SerialBuffer ::popBytes(U8* const addr, FwSizeType buffCapacity, FwSizeType n) {

// Check for null destination with a non-zero pop count and buffCapacity
if (addr == nullptr && n > 0) {
return FW_DESERIALIZE_SIZE_MISMATCH;
}

if (n > buffCapacity) {
return FW_DESERIALIZE_SIZE_MISMATCH;
}

return this->deserializeTo(addr, buffCapacity, n, Fw::Serialization::OMIT_LENGTH);
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
}

} // namespace Fw
5 changes: 3 additions & 2 deletions Fw/Types/SerialBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,9 @@ class SerialBuffer final : public LinearBufferBase {
);

//! Pop n bytes off the buffer
SerializeStatus popBytes(U8* const addr, //!< Address of bytes to pop
FwSizeType n //!< Number of bytes to pop
SerializeStatus popBytes(U8* const addr, //!< Address of bytes to pop
FwSizeType buffCapacity, //!< Capacity in bytes of addr
FwSizeType n //!< Number of bytes to pop
);

private:
Expand Down
53 changes: 34 additions & 19 deletions Fw/Types/Serializable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -515,14 +515,33 @@
return FW_SERIALIZE_OK;
}

SerializeStatus LinearBufferBase::deserializeTo(U8* buff, Serializable::SizeType& length, Endianness endianMode) {
SerializeStatus LinearBufferBase::deserializeTo(U8* buff,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
FwSizeType buffCapacity,
Serializable::SizeType& length,
Endianness endianMode) {

// validate arguments
//

Check warning

Code scanning / CodeQL

Unchecked function argument Warning

This use of parameter buffCapacity has not been checked.
// A non-zero capacity with a null destination pointer would reach memcpy
// with an invalid address. A null pointer is only safe when nothing will
// be copied (buffCapacity == 0).
if (buff == nullptr && buffCapacity > 0) {
return FW_DESERIALIZE_SIZE_MISMATCH;
}

// endianMode is an enum; an out-of-range value indicates a caller bug.
FW_ASSERT(endianMode == Endianness::BIG || endianMode == Endianness::LITTLE,
static_cast<FwAssertArgType>(endianMode));

FwSizeType length_in_out = static_cast<FwSizeType>(length);
SerializeStatus status = this->deserializeTo(buff, length_in_out, Serialization::INCLUDE_LENGTH, endianMode);
SerializeStatus status =
this->deserializeTo(buff, buffCapacity, length_in_out, Serialization::INCLUDE_LENGTH, endianMode);
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
length = static_cast<Serializable::SizeType>(length_in_out);
return status;
}

SerializeStatus LinearBufferBase::deserializeTo(U8* buff,
FwSizeType buffCapacity,
Serializable::SizeType& length,
Serialization::t lengthMode,
Endianness endianMode) {
Expand All @@ -537,8 +556,8 @@
return stat;
}

// make sure it fits
if ((storedLength > this->getDeserializeSizeLeft()) or (storedLength > length)) {
// make sure it fits in both the source and the destination
if ((storedLength > this->getDeserializeSizeLeft()) or (storedLength > buffCapacity)) {
return FW_DESERIALIZE_SIZE_MISMATCH;
}

Expand All @@ -550,11 +569,16 @@
length = static_cast<FwSizeType>(storedLength);

} else {
// make sure enough is left
// make sure enough is left in the source
if (length > this->getDeserializeSizeLeft()) {
return FW_DESERIALIZE_SIZE_MISMATCH;
}

// make sure the destination buffer is large enough
if (length > buffCapacity) {
return FW_DESERIALIZE_SIZE_MISMATCH;
}

if (length > 0) {
FW_ASSERT(buff);
}
Expand Down Expand Up @@ -930,19 +954,10 @@
return this->deserializeTo(val);
}

// Deprecated method for backward compatibility
SerializeStatus LinearBufferBase::deserialize(U8* buff, FwSizeType& length, bool noLength) {
const Serialization::t mode = noLength ? Serialization::OMIT_LENGTH : Serialization::INCLUDE_LENGTH;
return this->deserializeTo(buff, length, mode);
}

SerializeStatus LinearBufferBase::deserialize(U8* buff, FwSizeType& length) {
return this->deserializeTo(buff, length, Serialization::INCLUDE_LENGTH);
}

SerializeStatus LinearBufferBase::deserialize(U8* buff, FwSizeType& length, Serialization::t mode) {
return this->deserializeTo(buff, length, mode);
}
// NOTE: The deprecated deserialize(U8* buff, ...) overloads have been removed.
// They could not be safely forwarded to deserializeTo() after the addition of
// the buffCapacity parameter. Callers must migrate to:
// deserializeTo(buff, buffCapacity, length, lengthMode)

SerializeStatus LinearBufferBase::deserialize(Serializable& val) {
return this->deserializeTo(val);
Expand All @@ -955,4 +970,4 @@
return this->getCapacity();
}

} // namespace Fw
} // namespace Fw

Check warning

Code scanning / CppCheck

Could not find a newline character at the end of the file. Warning

Could not find a newline character at the end of the file.
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
28 changes: 21 additions & 7 deletions Fw/Types/Serializable.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -456,10 +456,14 @@ class SerialBufferBase {
//! deserialization can be controlled via the endianMode parameter.
//!
//! \param buff Pointer to the buffer where deserialized data will be stored
//! \param buffCapacity Size in bytes of the destination buffer pointed to by buff
//! \param length Reference to store the actual number of bytes deserialized
//! \param endianMode Endianness mode for deserialization (default is Endianness::BIG)
//! \return SerializeStatus indicating the result of the operation
virtual SerializeStatus deserializeTo(U8* buff, FwSizeType& length, Endianness endianMode = Endianness::BIG) = 0;
virtual SerializeStatus deserializeTo(U8* buff,
FwSizeType buffCapacity,
FwSizeType& length,
Endianness endianMode = Endianness::BIG) = 0;

//! \brief Deserialize a byte buffer with optional length prefix
//!
Expand All @@ -470,11 +474,13 @@ class SerialBufferBase {
//! endianMode parameter.
//!
//! \param buff Pointer to the buffer where deserialized data will be stored
//! \param buffCapacity Size in bytes of the destination buffer pointed to by buff
//! \param length Reference to store the actual number of bytes deserialized
//! \param lengthMode Specifies whether length was included in serialization (INCLUDE_LENGTH or OMIT_LENGTH)
//! \param endianMode Endianness mode for deserialization (default is Endianness::BIG)
//! \return SerializeStatus indicating the result of the operation
virtual SerializeStatus deserializeTo(U8* buff,
FwSizeType buffCapacity,
FwSizeType& length,
Serialization::t lengthMode,
Endianness endianMode = Endianness::BIG) = 0;
Expand Down Expand Up @@ -1031,10 +1037,14 @@ class LinearBufferBase : public SerialBufferBase {
//! deserialization can be controlled via the endianMode parameter.
//!
//! \param buff Pointer to the buffer where deserialized data will be stored
//! \param buffCapacity Size in bytes of the destination buffer pointed to by buff
//! \param length Reference to store the actual number of bytes deserialized
//! \param endianMode Endianness mode for deserialization (default is Endianness::BIG)
//! \return SerializeStatus indicating the result of the operation
SerializeStatus deserializeTo(U8* buff, FwSizeType& length, Endianness endianMode = Endianness::BIG) override;
SerializeStatus deserializeTo(U8* buff,
FwSizeType buffCapacity,
FwSizeType& length,
Endianness endianMode = Endianness::BIG) override;

//! \brief Deserialize a byte buffer with optional length prefix
//!
Expand All @@ -1045,11 +1055,13 @@ class LinearBufferBase : public SerialBufferBase {
//! endianMode parameter.
//!
//! \param buff Pointer to the buffer where deserialized data will be stored
//! \param buffCapacity Size in bytes of the destination buffer pointed to by buff
//! \param length Reference to store the actual number of bytes deserialized
//! \param lengthMode Specifies whether length was included in serialization (INCLUDE_LENGTH or OMIT_LENGTH)
//! \param endianMode Endianness mode for deserialization (default is Endianness::BIG)
//! \return SerializeStatus indicating the result of the operation
SerializeStatus deserializeTo(U8* buff,
FwSizeType buffCapacity,
FwSizeType& length,
Serialization::t lengthMode,
Endianness endianMode = Endianness::BIG) override;
Expand Down Expand Up @@ -1322,12 +1334,14 @@ class LinearBufferBase : public SerialBufferBase {
DEPRECATED(SerializeStatus deserialize(F64& val), "Use deserializeTo(F64& val) instead");
DEPRECATED(SerializeStatus deserialize(bool& val), "Use deserializeTo(bool& val) instead");
DEPRECATED(SerializeStatus deserialize(void*& val), "Use deserializeTo(void*& val) instead");
DEPRECATED(SerializeStatus deserialize(U8* buff, FwSizeType& length, bool noLength),
"Use deserialize(U8* buff, FwSizeType& length, Serialization::t mode) instead");
DEPRECATED(
SerializeStatus deserialize(U8* buff, FwSizeType& length, bool noLength),
"Use deserializeTo(U8* buff, FwSizeType buffCapacity, FwSizeType& length, Serialization::t mode) instead");
DEPRECATED(SerializeStatus deserialize(U8* buff, FwSizeType& length),
"Use deserializeTo(U8* buff, FwSizeType& length) instead");
DEPRECATED(SerializeStatus deserialize(U8* buff, FwSizeType& length, Serialization::t mode),
"Use deserializeTo(U8* buff, FwSizeType& length, Serialization::t mode) instead");
"Use deserializeTo(U8* buff, FwSizeType buffCapacity, FwSizeType& length) instead");
DEPRECATED(
SerializeStatus deserialize(U8* buff, FwSizeType& length, Serialization::t mode),
"Use deserializeTo(U8* buff, FwSizeType buffCapacity, FwSizeType& length, Serialization::t mode) instead");
DEPRECATED(SerializeStatus deserialize(Serializable& val), "Use deserializeTo(Serializable& val) instead");

#ifdef BUILD_UT
Expand Down
12 changes: 11 additions & 1 deletion Fw/Types/StringBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,20 @@ SerializeStatus StringBase::deserializeFrom(SerialBufferBase& buffer, Fw::Endian
// Public interface returns const char*, but implementation needs char*
// So use const_cast
CHAR* raw = const_cast<CHAR*>(this->toChar());

// CodeQL fix: validate the source buffer before forwarding it to
// deserializeTo. SerialBufferBase does not expose getBuffAddr(), so we
// check getCapacity() — a zero-capacity buffer has no usable backing
// storage and any read from it would be undefined behaviour.
if (buffer.getCapacity() == 0) {
return FW_DESERIALIZE_SIZE_MISMATCH;
}

// Deserialize length
// Fail if length exceeds max size (the initial value of actualSize)
// Otherwise deserialize length bytes and set actualSize to length
SerializeStatus stat = buffer.deserializeTo(reinterpret_cast<U8*>(raw), actualSize, Serialization::INCLUDE_LENGTH);
SerializeStatus stat = buffer.deserializeTo(reinterpret_cast<U8*>(raw), static_cast<FwSizeType>(maxSize),
actualSize, Serialization::INCLUDE_LENGTH);
if (stat == FW_SERIALIZE_OK) {
// Deserialization succeeded: null-terminate string at actual size
FW_ASSERT(actualSize <= maxSize, static_cast<FwAssertArgType>(actualSize),
Expand Down
2 changes: 1 addition & 1 deletion Ref/RecvBuffApp/RecvBuffComponentImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ void RecvBuffImpl::Data_handler(FwIndexType portNum, Drv::DataBuffer& buff) {
// deserialize data
U8 testData[24] = {0};
FwSizeType size = sizeof(testData);
stat = buff.deserializeTo(testData, size);
stat = buff.deserializeTo(testData, sizeof(testData), size, Fw::Serialization::OMIT_LENGTH);
FW_ASSERT(stat == Fw::FW_SERIALIZE_OK, static_cast<FwAssertArgType>(stat));
// deserialize checksum
U32 csum = 0;
Expand Down
2 changes: 2 additions & 0 deletions Svc/Ccsds/SpacePacketFramer/SpacePacketFramer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ void SpacePacketFramer ::dataIn_handler(FwIndexType portNum, Fw::Buffer& data, c
FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
status = frameSerializer.serializeFrom(data.getData(), data.getSize(), Fw::Serialization::OMIT_LENGTH);
FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status);
// Trim to actual frame size in case allocator returned a larger buffer
frameBuffer.setSize(static_cast<Fw::Buffer::SizeType>(frameSize));

this->dataOut_out(0, frameBuffer, context);
this->dataReturnOut_out(0, data, context); // return ownership of the original data buffer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ TEST(SpacePacketFramer, testNominalFraming) {
tester.testNominalFraming();
}

TEST(SpacePacketFramer, OversizedAllocatorBufferIsTrimmed) {
Svc::Ccsds::SpacePacketFramerTester tester;
tester.testOversizedAllocatorBufferIsTrimmed();
}

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