pinMode() does not check valid pin number

[mikaelpatel]

The current pinMode() implementation performs a digitalPinToPort(pin) and then checks the port for NOT_A_PORT but digitalPinToPort() is a program memory table read. It is possible to give an illegal pin number.

Possible correction is:

void pinMode(uint8_t pin, uint8_t mode)
{
    if (pin >= NUM_DIGITAL_PINS) return;
    uint8_t bit = digitalPinToBitMask(pin);
    uint8_t port = digitalPinToPort(pin);
    volatile uint8_t *reg, *out;
    ...
}

Replacing the port check with a pin number check (first).

Cheers!

[cousteaulecommandant]

This also affects digitalRead() and digitalWrite().

Honestly I have no idea how this check was supposed to work. It would have made sense if pins_arduino.h defined digital_pin_to_port_PGM as something like

const uint8_t PROGMEM digital_pin_to_port_PGM[256] = {
    /*  0 -  7 */ PD, PD, PD, PD, PD, PD, PD, PD, 
    /*  8 - 14 */ PB, PB, PB, PB, PB, PB, 
    /* 15 - 19 */ PC, PC, PC, PC, PC, PC
    /* 20 - 255: all zeros (NOT_A_PIN) */
};

so that if you queried it with a (uint8_t) pin from 20 to 255 you'd get 0, thus failing the if (port == NOT_A_PIN) return; check. But this is not the case since there's no [256] on the code, so trying to fetch any pin past that would either return "garbage values" or trigger some sort of undefined behavior.
(Fixing this by adding the 256 makes no sense though; that'd involve wasting a lot of memory. @mikaelpatel's solution makes much more sense.)

[per1234]

What's the worst case result of passing an invalid pin number to one of these functions using Arduino AVR Boards if no checks at all were done?

Is it possible this could cause hardware damage on a board with no external connections or corrupt the firmware? If the answer is no then I think it would be best to just assume that the user is passing a valid pin number and remove the overhead of checking the validity of the value, including the current NOT_A_PIN check. If the user's code is messed up badly enough to be passing invalid pin values then they're going to be experiencing unexpected behavior regardless of any sanity checks so this provides no clear benefit and only adds overhead.

[cousteaulecommandant]

What's the worst case result of passing an invalid pin number to one of these functions using Arduino AVR Boards if no checks at all were done?

Well, looking up outside of the lookup table is undefined behavior, so in principle anything can happen.
In practice I don't think there would be electrical problems from the chip's side; outside of it it depends on what's connected but that could also cause trouble even without the check if the user enters the wrong pin number, so the check doesn't provide any guarantee.
In any case, I'd either do this check right or not do it at all, rather than leaving it as it is now. (Doing it right sounds safer, but I guess Arduino is "in a budget" when it comes to code size and speed.)

[neu-rah]

#2

[aentinger]

@mikaelpatel Good catch 👍 Could create a small PR for this?

[cousteaulecommandant]

@mikaelpatel Good catch +1 Could create a small PR for this?

Back in the day I opened arduino/Arduino#4649, but it was closed after Chainsaw and never reopened.
Now there is #302 targeting the same issue (not sure how different it is or which approach is better, although I do remember my approach was more of a hack than a proper fix).