ARMmbed / mbed-drivers

Drivers for common MCU peripherals in mbed OS.
Other
39 stars 42 forks source link

FunctionPointerBase::call() no check for valid if fptr has been set #83

Open 0xc0170 opened 9 years ago

0xc0170 commented 9 years ago

FunctionPointerBase method call()

// taken from FunctionPointerBase.h
    inline R call(void* arg) {
        return _membercaller(_object, _member, arg);
    }

does not check if _membercaller or _object are valid.

rainierwolfcastle commented 9 years ago

ARM Internal Ref: IOTSFW-1012

bremoran commented 8 years ago

What is the expected failure mode? What is a valid pointer?

In the C world, if you call an invalid Function Pointer, the results are either a null-pointer dereference (if it was 0), an alignment error (if (ptr & 3)), an undefined instruction exception (if the pointer was to a non-code segment and you're lucky) or undefined results (if the pointer was to a non-code segment and you're unlucky).

Ideally, I'd like to see two system APIs to deal with this problem:

pointerValid(uint8_t pmask, void *ptr);
pointerException(void *ptr);

pointerValid() would validate against ACLs provided by the uvisor, which would validate whether a given pointer is valid for read, write, or execute from the current context.

pointerException() would breakpoint if a debugger is connected, invoke the uvisor's exception handling mechanism if the uvisor is active, or log and reset if neither of the above is true.

Otherwise, the current state of things is exactly what you expect from function pointers.

bremoran commented 8 years ago

cc @AlessandroA @meriac

meriac commented 8 years ago

We're planning to create a generic debug delegation API for uvisor:

https://github.com/ARMmbed/uvisor/issues/70

This will allow uVisor and other system components to submit sophisticated debug messages without understanding where they end up (NV-logging, mesh network, Ethernet, USB etc.). The "drop-dead" functions is included as well - currently we have a blinkcode for fault-reason.

0xc0170 commented 8 years ago

Valid in the above context was meant to be as they are set to not NULL, not to point to nothing as stated in the FunctionPointerBase.

FunctionPointerBase provides the method clear(), similar to what its ctor does - set both variables referenced in the report to NULL. Calling call() causes then null-pointer dereference. I recall in the old FunctionPointer there was a check present in the call() method.

bremoran commented 8 years ago

Sure, and while there's a small performance overhead for that check, it's something we could disable, possibly with yotta config. What I still don't have is an expected failure mode. An assert is probably the only thing we can do at the moment.

Clearing a function pointer, then calling it is still no worse than:

void (*foo)() = NULL;
foo();

While it's great that we can perform a check, I want to make sure that the result of performing the check is meaningful. That means: assert, breakpoint, watchdog reset, halt-and-catch-fire... There needs to be a meaningful failure mode.

bremoran commented 8 years ago

The point of the non-null pointer failure modes was to demonstrate that simply checking for NULL is probably not enough to guarantee safety when invoking FunctionPointerBase::call()