Potential corruption in attachInterrupt()

[NormanDunbar]

The Arduino reference for attachInterrupt() states, in the syntax section, that this function should be called as per:

attachInterrupt(digitalPinToInterrupt(pin), ISR, mode); (recommended)

However, digitalPinToInterrupt() returns NOT_AN_INTERRUPT (or -1) if a pin other than 2 or 3 is passed. Within the code for attachInterrupt() in WInterrupts.c, I see this:

void attachInterrupt(uint8_t interruptNum, void (*userFunc)(void), int mode) {
  if(interruptNum < EXTERNAL_NUM_INTERRUPTS) {
      intFunc[interruptNum] = userFunc;

So, if called as recommended, and an invalid pin is passed, the -1is indeed less than EXTERNAL_NUM_INTERRUPTS (or 2) and so the access to the array is out of bounds and may corrupt whatever is in memory just before the array.

I suggest the following should (!) fix it:

 if(interruptNum < EXTERNAL_NUM_INTERRUPTS &&
    interruptNum >= 0) {
      intFunc[interruptNum] = userFunc;

That way, NOT_AN_INTERRUPT will be ignored. Just for safety, I would probably do a similar edit to detachInterrupt() as it doesn't validate the incoming interrupt number for NOT_AN_INTERRUPT either.

Cheers.

[facchinm]

Hi @NormanDunbar ,
would you mind opening a PR with your modifications? Thanks!

[NormanDunbar]

Hi @facchinm ,

yes, no problem, I'll do that soon.

Cheers,
Norm.

[aweatherguy]

I'm a bit late to the party here -- was perusing these issues and noticed something about the proposed fix. I don't think a fix is required, or if so what it should be.

Here's why: the interrupt number argument to attachInterrupt() is of type uint8_t -- unsigned. It is not possible for an unsigned value to be less than zero. If you code this:

attachInterupt( -1, .... )

The actual value passed to the function is not a signed -1, but an unsigned 255. Also, if you try to compile this:

void func( uint8_t x )
{
    if ( (x < 5) && (x >= 0)) { ... do something ... }

The compiler knows that (x >= 0) is always true because x is unsigned. You will get this warning from the compiler:

error: comparison is always true due to limited range of data type [-Werror=type-limits]

Anyway, I suggest that no fix is required because NOT_AN_INTERRUPT will get passed as 255 to attachInterrupt and that will fail the validity test as is.

[NormanDunbar]

Hmm. Sounds plausible. I'll check when I'm next at my computer. I'm working away at present.

Cheers,
Norm.

[aweatherguy]

I've been thinking more about a "fix". I suppose that even though the current design works, one could argue that it's not the best programming practice to pass a negative value as an unsigned function argument (i.e. NOT_AN_INTERRUPT = -1). As evidenced by the confusion it has caused if nothing else.

I did a search on the Arduino 1.8.5 release and the only place that macro is used is in the pins_arduino.h file of multiple "variants" directories. It's possible it is also used in contributed libraries but I'm not sure how one would search all of that.

Perhaps one "fix" would be to re-define the macro as "255" instead of "-1". I don't think it would break anything in pins_arduino.h. Playing devil's advocate, if a sketch or library assigned the return from digitalPinToInterrupt() to a signed integer then tested to see if it was negative, this fix would break that. That would technically be the fault of the sketch/library in not comparing the result against NOT_AN_INTERRUPT, but that's not a customer friendly approach.

Maybe best to just let sleeping dogs lie...? I don't feel strongly one way or the other on this. Others may have stronger feelings about if or how to fix this.

I mainly chimed in to point out the issue with -1 being passed as an unsigned value. Sorry I don't have a more solid recommendation.

[pratikpc]

@aweatherguy @facchinm @NormanDunbar I would favour moving towards a Modern C++ based solution
Modern C++ of this level is already supported by Arduino IDE via the GCC compiler.

For example, in an Uno, the Macro could be replaced by the following function

constexpr uint8_t digitalPinToInterrupt(uint8_t const p)
{
    return  ((p) == 2 ? 0 : ((p) == 3 ? 1 : NOT_AN_INTERRUPT));
}

constexpr would ensure that this can end up being a Compile Time Constant solution if required.
The uint8_t fixed return type would ensure that the returned value is between 0-255 irrespective of the destination type.

For example, check out a short example on CompilerExplorer

This would solve all confusions we have

[NormanDunbar]

Morning @pratikpc @aweatherguy @facchinm,

the problem isn't with digitalPinToInterrupt() returning -1, which it does for NOT_AN_INTERRUPT, it's with attachInterrupt() taking an unsigned 8 bit integer as the interrupt number. The way the documentation web site says to use these functions is attachInterrupt(digitalPinToInterrupt(pin),...) so confusion reigns.

In my case because I didn't properly read the header for attachInterrupt() to notice that it was unsigned, and figured that if it got passed NOT_AN_INTERRUPT it would corrupt whatever was in memory just before the table where it keeps interrupt handling functions.

So, given that NOT_AN_INTERRUPT is "converted" from -1 to 255, it wouldn't pass the test if(interruptNum < EXTERNAL_NUM_INTERRUPTS) so, while the "bug" I first thought was a problem is not going to happen, which is good, it was certainly confusing - hence the continuing discussions.

I think the correct (for certain values of 'correct') solution is to make attachInterrupt() take a signed 8 bit integer and specifically test for the pin number being >0 and < EXTERNAL_NUM_INTERRUPTS before processing any further.

Other opinions are of course welcome.

Cheers,
Norm.

[pratikpc]

@NormanDunbar I don't think taking a signed value is indeed a good idea.
It's just a waste of 4 bits given a Pin Number practically should not be negative.

[NormanDunbar]

@pratikpc That's fine. However, if that's the case, then digitalPinToInterrupt() should not return a signed value. So one of the two functions must change in order to be "correct".

I'm not sure where you get "4 bits" wasted from though. In an 8 bit signed int, the sign takes one bit, bit 7. The range of values is shifted down by 128 from that of an unsigned 8 bit int. (-128 to 127 as opposed to 0 to 255) so you still get 256 different values.

We agree that pin numbers cannot be negative, which is (probably) why, NOT_AN_INTERRUPT is negative, to indicate a completely erroneous situation. The problem is, when it gets into attachInterrupt() is is positive and could represent a valid pin number, if no checking was done. And indeed, what if Microchip devide to create the MeagMegaMegaAVR with 1024 pins? Suddenly a positive NOT_AN_INTERRUPT is a problem. :-(

It shouldn't be allowed to pass unsigned to signed and vice versa - my gcc/g++ compiler gives me warnings when I do that in other systems. Granted, warnings can be turned off, but to be honest, they are best left on - just in case I do something silly!

Cheers,
Norm.

PS. Where are you in India? I'm just back from Kerala, a beautiful place.

[pratikpc]

I support making digitalPinToInterrupt return unsigned values always.
It can be easily done by either adding type-casting in front within the Macro or making it a C++( or C) function. IMHO a C or C++ Function would result in a much cleaner code base.

In order to deal with 1024 Pins, although that's another story irrespective of feasibility we could use a Pin type(typedefed to uint8_t or uint16_t)

For example, for a regular Uno to save bits
using Pin = uint8_t;
However for MeagMegaMegaAVR we could use something like
using Pin = uint16_t;
Now, each and every function would take a Pin as a parameter
digitalRead(Pin pin);
digitalWrite(Pin pin, byte value);

Now in such a scenario for MeagMegaMegaAVR given our digitalPinToInterrupt returns -1 on no pin, it would get casted to UINT16_MAX thus again averting disaster.

PS. Where are you in India? I'm just back from Kerala, a beautiful place.

I live in Bombay. Kerala is a really beautiful place. I wish I could go there once again. Hope you enjoyed your journey!