Fixing review comments - pt 1

LeStarch · Feb 27, 2025 · d636bbf · d636bbf
1 parent 1928a00
commit d636bbf
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 27 deletions.
diff --git a/Os/ValidateFileCommon.cpp b/Os/ValidateFileCommon.cpp
@@ -78,7 +78,7 @@ namespace Os {
         if( File::OP_OK != status ) {
             return status;
         }
-        if( size != static_cast<FwSignedSizeType>(hashBuffer.getBuffCapacity()) ) {
+        if(static_cast<FwSizeType>(size) != hashBuffer.getBuffCapacity()) {
             return File::BAD_SIZE;
         }
         hashFile.close();
@@ -105,7 +105,7 @@ namespace Os {
         if( File::OP_OK != status ) {
             return status;
         }
-        if( size != static_cast<FwSignedSizeType>(hashBuffer.getBuffLength()) ) {
+        if(static_cast<FwSizeType>(size) != hashBuffer.getBuffLength()) {
             return File::BAD_SIZE;
         }
         hashFile.close();

diff --git a/Svc/ActiveTextLogger/LogFile.cpp b/Svc/ActiveTextLogger/LogFile.cpp
@@ -68,7 +68,7 @@ namespace Svc {
                 FW_ASSERT(stat != Os::File::NOT_OPENED);
 
                 // Only return a good status if the write was valid
-                status = (static_cast<FwSizeType>(writeSize) == size);
+                status = (stat == Os::File::OP_OK) && (static_cast<FwSizeType>(writeSize) == size);
 
                 this->m_currentFileSize += static_cast<FwSizeType>(writeSize);
             }

diff --git a/Svc/BufferAccumulator/BufferAccumulator.cpp b/Svc/BufferAccumulator/BufferAccumulator.cpp
@@ -15,7 +15,7 @@
 #include <sys/time.h>
 
 #include "Fw/Types/BasicTypes.hpp"
-
+#include <limits>
 
 namespace Svc {
 
@@ -46,15 +46,19 @@ BufferAccumulator ::~BufferAccumulator() {}
 // ----------------------------------------------------------------------
 
 void BufferAccumulator ::allocateQueue(
-    NATIVE_INT_TYPE identifier, Fw::MemAllocator& allocator,
+    FwEnumStoreType identifier, Fw::MemAllocator& allocator,
     NATIVE_UINT_TYPE maxNumBuffers  //!< The maximum number of buffers
 ) {
 
   this->m_allocatorId = identifier;
+  // Overflow protection
+  FW_ASSERT(
+      (std::numeric_limits<FwSizeType>::max() / maxNumBuffers) >= sizeof(Fw::Buffer)
+  );
   FwSizeType memSize = static_cast<FwSizeType>(sizeof(Fw::Buffer) * maxNumBuffers);
   bool recoverable = false;
   this->m_bufferMemory = static_cast<Fw::Buffer*>(
-      allocator.allocate(static_cast<FwEnumStoreType>(identifier), memSize, recoverable));
+      allocator.allocate(identifier, memSize, recoverable));
   //TODO: Fail gracefully here
   m_bufferQueue.init(this->m_bufferMemory, maxNumBuffers);
 }

diff --git a/Svc/BufferAccumulator/BufferAccumulator.hpp b/Svc/BufferAccumulator/BufferAccumulator.hpp
@@ -103,7 +103,7 @@ namespace Svc {
         //! Give the class a memory buffer. Should be called after constructor
         //! and init, but before task is spawned.
         void allocateQueue(
-                NATIVE_INT_TYPE identifier, Fw::MemAllocator& allocator,
+                FwEnumStoreType identifier, Fw::MemAllocator& allocator,
                 NATIVE_UINT_TYPE maxNumBuffers  //!< The maximum number of buffers
                 );
 
@@ -204,7 +204,7 @@ namespace Svc {
         U32 m_cmdSeq;
 
         //! The allocator ID
-        NATIVE_INT_TYPE m_allocatorId;
+        FwEnumStoreType m_allocatorId;
     };
 
 }  // namespace Svc

diff --git a/Utils/CRCChecker.cpp b/Utils/CRCChecker.cpp
@@ -34,7 +34,6 @@ static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING,
     Os::File::Status stat;
     Utils::Hash hash;
     U32 checksum;
-    FwSignedSizeType int_file_size;
     FwSignedSizeType bytes_to_read;
     FwSignedSizeType bytes_to_write;
     Fw::FileNameString hashFilename;
@@ -46,8 +45,6 @@ static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING,
       return FAILED_FILE_SIZE;
     }
 
-    int_file_size = filesize;
-
     // Open file
     stat = f.open(fname, Os::File::OPEN_READ);
     if(stat != Os::File::OP_OK)
@@ -57,7 +54,7 @@ static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING,
 
     // Read file
     bytes_to_read = CRC_FILE_READ_BLOCK;
-    blocks = int_file_size / CRC_FILE_READ_BLOCK;
+    blocks = filesize / CRC_FILE_READ_BLOCK;
     for(i = 0; i < blocks; i++)
     {
       stat = f.read(block_data, bytes_to_read);
@@ -70,7 +67,7 @@ static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING,
       hash.update(block_data, static_cast<FwSizeType>(bytes_to_read));
     }
 
-    remaining_bytes = int_file_size % CRC_FILE_READ_BLOCK;
+    remaining_bytes = filesize % CRC_FILE_READ_BLOCK;
     bytes_to_read = remaining_bytes;
     if(remaining_bytes > 0)
     {
@@ -158,7 +155,6 @@ static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING,
     Utils::Hash hash;
     U32 checksum;
     U32 checksum_from_file;
-    FwSignedSizeType int_file_size;
     FwSignedSizeType bytes_to_read;
     U8 block_data[CRC_FILE_READ_BLOCK];
 
@@ -168,12 +164,6 @@ static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING,
       return FAILED_FILE_SIZE;
     }
 
-    int_file_size = static_cast<FwSignedSizeType>(filesize);
-    if(static_cast<FwSignedSizeType>(int_file_size) != filesize)
-    {
-      return FAILED_FILE_SIZE_CAST;
-    }
-
     // Open file
     stat = f.open(fname, Os::File::OPEN_READ);
     if(stat != Os::File::OP_OK)
@@ -196,7 +186,7 @@ static_assert(FW_USE_PRINTF_FAMILY_FUNCTIONS_IN_STRING_FORMATTING,
       hash.update(block_data, static_cast<FwSizeType>(bytes_to_read));
     }
 
-    remaining_bytes = int_file_size % CRC_FILE_READ_BLOCK;
+    remaining_bytes = filesize % CRC_FILE_READ_BLOCK;
     bytes_to_read = remaining_bytes;
     if(remaining_bytes > 0)
     {

diff --git a/Utils/Types/Queue.cpp b/Utils/Types/Queue.cpp
@@ -22,7 +22,7 @@ void Queue::setup(U8* const storage, const FwSizeType storage_size, const FwSize
         static_cast<FwAssertArgType>(storage_size),
         static_cast<FwAssertArgType>(depth),
         static_cast<FwAssertArgType>(message_size));
-    m_internal.setup(storage, static_cast<FwSizeType>(total_needed_size));
+    m_internal.setup(storage, total_needed_size);
     m_message_size = message_size;
 }
 
@@ -32,7 +32,7 @@ Fw::SerializeStatus Queue::enqueue(const U8* const message, const FwSizeType siz
         m_message_size == size,
         static_cast<FwAssertArgType>(size),
         static_cast<FwAssertArgType>(m_message_size)); // Message size is as expected
-    return m_internal.serialize(message, static_cast<FwSizeType>(m_message_size));
+    return m_internal.serialize(message, m_message_size);
 }
 
 Fw::SerializeStatus Queue::dequeue(U8* const message, const FwSizeType size) {
@@ -41,16 +41,16 @@ Fw::SerializeStatus Queue::dequeue(U8* const message, const FwSizeType size) {
         m_message_size <= size,
         static_cast<FwAssertArgType>(size),
         static_cast<FwAssertArgType>(m_message_size)); // Sufficient storage space for read message
-    Fw::SerializeStatus result = m_internal.peek(message, static_cast<FwSizeType>(m_message_size), 0);
+    Fw::SerializeStatus result = m_internal.peek(message, m_message_size, 0);
     if (result != Fw::FW_SERIALIZE_OK) {
         return result;
     }
-    return m_internal.rotate(static_cast<FwSizeType>(m_message_size));
+    return m_internal.rotate(m_message_size);
 }
 
 FwSizeType Queue::get_high_water_mark() const {
     FW_ASSERT(m_message_size > 0, static_cast<FwAssertArgType>(m_message_size));
-    return static_cast<FwSizeType>(m_internal.get_high_water_mark() / m_message_size);
+    return m_internal.get_high_water_mark() / m_message_size;
 }
 
 void Queue::clear_high_water_mark() {
@@ -59,7 +59,7 @@ void Queue::clear_high_water_mark() {
 
 FwSizeType Queue::getQueueSize() const {
     FW_ASSERT(m_message_size > 0, static_cast<FwAssertArgType>(m_message_size));
-    return static_cast<FwSizeType>(m_internal.get_allocated_size() / m_message_size);
+    return m_internal.get_allocated_size() / m_message_size;
 }