nasa / fprime

F´ - A flight software and embedded systems framework
https://fprime.jpl.nasa.gov
Apache License 2.0
10.06k stars 1.31k forks source link

Port number validation does not check for negative port numbers #2966

Open celskeggs opened 4 days ago

celskeggs commented 4 days ago
F´ Version 65b9e4d9d55db357f4478afb8f9ed5b14786c904
Affected Component FPP autocoder

Problem Description

Autocoded base class functions contain a FW_ASSERT to check that the specified port number is in range. The index type is signed, but the comparison only checks for a maximum. This means that a negative number would not be caught.

Example:

    MyType MyComponentComponentBase ::
      myport_handlerBase(FwIndexType portNum)
    {
      // Make sure port number is valid
      FW_ASSERT(
        portNum < this->getNum_myport_InputPorts(),
        static_cast<FwAssertArgType>(portNum)
      );

      MyType retVal;

      // Call handler function
      retVal = this->myport_handler(portNum);

      return retVal;
    }

Context / Environment

Execute fprime-util version-check and share the output.

Operating System: Linux
CPU Architecture: x86_64
Platform: Linux-5.15.153.1-microsoft-standard-WSL2-x86_64-with-glibc2.34
Python version: 3.9.18
CMake version: 3.26.5
Pip version: 21.2.3
Pip packages:
    fprime-tools==3.4.5a1
    fprime-gds==3.4.4a3
    fprime-fpp-*==2.2.0a5

How to Reproduce

  1. Look at any autocoded base class

Expected Behavior

FW_ASSERT should be used to check for negative portNum values and not just positive portNum values.

0 <= portNum && portNum < this->getNum_myport_InputPorts()
LeStarch commented 4 days ago

Agreed. This should be fixed.

LeStarch commented 4 days ago

@timcanham functionally this is an index into the port array and is unsigned. Is there a reason to keep this type signed, other than the historical usage of NATIVE_INT_TYPE before the use of FwIndexType?

timcanham commented 4 days ago

No reason... We can change it, but it would be a breaking fix for existing users' implementation classes.