canonical / matter-pi-gpio-commander

Matter Raspberry Pi GPIO Commander - Turn your Pi into a Matter lighting device!
Apache License 2.0
94 stars 2 forks source link

Exception handling for GPIO number conversion #11

Closed farshidtz closed 1 year ago

farshidtz commented 1 year ago

The stoi function is used for converting the value of GPIO environment variable to an integer. When a non-integer is set for the value, the application crashes:

$ sudo snap set matter-pi-gpio-commander gpio=x
$ sudo snap run matter-pi-gpio-commander.lighting
...
[1677659847.179663][14576:14576] CHIP:SVR: Using GPIO x
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoi
/snap/matter-pi-gpio-commander/x7/bin/run.sh: line 3: 14576 Aborted                 (core dumped) $SNAP/bin/lighting-app $ARGS

The test-blink app has exception handling for stoi: https://github.com/canonical/matter-pi-gpio-commander/blob/4df75fad2a661c2a93e73867909444b8cbce41bc/test-blink.cpp#L24-L33

_Originally posted by @farshidtz in https://github.com/canonical/matter-pi-gpio-commander/pull/9#discussion_r1120206407_

MonicaisHer commented 1 year ago

Throwing exceptions has been disabled by the -fno-exceptions flag in SDK, resulting in the following build errors:

try
{
    gpio = std::stoi(envGPIO);
    ChipLogProgress(AppServer, "Using GPIO %d", gpio);
}
catch (const std::exception& ex)
{
    ChipLogError(AppServer, "Non-integer value for GPIO: %s", ex.what());
    return CHIP_ERROR_INVALID_ARGUMENT;
}
../../src/LightingManager.cpp:51:34: error: exception handling disabled, use ‘-fexceptions’ to enable
   51 |     catch (const std::exception& ex)
      |                                  ^~
In file included from ../../src/LightingManager.cpp:21:
../../src/LightingManager.cpp:53:67: error: ‘ex’ was not declared in this scope
   53 |         ChipLogError(AppServer, "Non-integer value for GPIO: %s", ex.what());

Here is a potential solution:

bool stoi_no_throw(const std::string& s, int& outResult) {
    size_t idx = 0;
    outResult = std::stoi(s, &idx);
    return idx == s.length();
}

CHIP_ERROR LightingManager::Init()
{
    mState = kState_On;

    char *envGPIO = std::getenv(GPIO);
    if (envGPIO == NULL || strlen(envGPIO) == 0)
    {
        ChipLogError(AppServer, "Environment variable not set or empty: %s", GPIO);
        return CHIP_ERROR_INVALID_ARGUMENT;
    }

    if (!stoi_no_throw(envGPIO, gpio))
    {
        ChipLogError(AppServer, "Non-integer value for GPIO: %s", envGPIO);
        return CHIP_ERROR_INVALID_ARGUMENT;
    }

    ChipLogProgress(AppServer, "Using GPIO %d", gpio);
    wiringPiSetupGpio();
    pinMode(gpio, OUTPUT);

    return CHIP_NO_ERROR;
}
farshidtz commented 1 year ago

Alternatively, we could find another library which doesn't throw exceptions for string to int conversion; see https://stackoverflow.com/a/24786330/3041544 and https://cplusplus.com/reference/cstdlib/atoi/

We could also look into enabling exception handling and make it possible to use modern libraries that use it. It should be fine doing so since this application is meant to run on Linux and can cope with the extra overhead.

Before detailing the library support for -fno-exceptions, first a passing note on the things lost when this flag is used: it will break exceptions trying to pass through code compiled with -fno-exceptions whether or not that code has any try or catch constructs. If you might have some code that throws, you shouldn't use -fno-exceptions. If you have some code that uses try or catch, you shouldn't use -fno-exceptions.

And what it to be gained, tinkering in the back alleys with a language like this? Exception handling overhead can be measured in the size of the executable binary, and varies with the capabilities of the underlying operating system and specific configuration of the C++ compiler. On recent hardware with GNU system software of the same age, the combined code and data size overhead for enabling exception handling is around 7%. Of course, if code size is of singular concern than using the appropriate optimizer setting with exception handling enabled (ie, -Os -fexceptions) may save up to twice that, and preserve error checking.

https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_exceptions.html

farshidtz commented 1 year ago

Closing since the gpio value now gets validated during configuration: https://github.com/canonical/matter-pi-gpio-commander/pull/13