Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added bounds checking on pinMode #302

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added bounds checking on pinMode #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d4c261
Added bounds to checking in digitalPinTo* functions
BarryPSmith Nov 30, 2019
7b336e1
Removed superflous consts. Fixed digital_pin_count checks.
BarryPSmith Dec 13, 2019
2b74a1e
Removed unnecessary declaration from pins_arduino.h
BarryPSmith Dec 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions cores/arduino/Arduino.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,24 +169,51 @@ extern const uint8_t PROGMEM digital_pin_to_port_PGM[];
// extern const uint8_t PROGMEM digital_pin_to_bit_PGM[];
extern const uint8_t PROGMEM digital_pin_to_bit_mask_PGM[];
extern const uint8_t PROGMEM digital_pin_to_timer_PGM[];
extern const uint8_t digital_pin_count;

#define NOT_A_PIN 0
#define NOT_A_PORT 0

#define NOT_AN_INTERRUPT -1

// Get the bit location within the hardware port of the given virtual pin.
// This comes from the pins_*.c file for the active board configuration.
//
// These perform slightly better as macros compared to inline functions
//
#define digitalPinToPort(P) ( pgm_read_byte( digital_pin_to_port_PGM + (P) ) )
#define digitalPinToBitMask(P) ( pgm_read_byte( digital_pin_to_bit_mask_PGM + (P) ) )
#define digitalPinToTimer(P) ( pgm_read_byte( digital_pin_to_timer_PGM + (P) ) )
//
#define analogInPinToBit(P) (P)
#define portOutputRegister(P) ( (volatile uint8_t *)( pgm_read_word( port_to_output_PGM + (P))) )
#define portInputRegister(P) ( (volatile uint8_t *)( pgm_read_word( port_to_input_PGM + (P))) )
#define portModeRegister(P) ( (volatile uint8_t *)( pgm_read_word( port_to_mode_PGM + (P))) )

#define NOT_A_PIN 0
#define NOT_A_PORT 0

#define NOT_AN_INTERRUPT -1
//
// bounds checking on these functions because they return a pointer to an address that will be modified.
// An out of bounds write can result in stack corruption and fun to track-down errors.
//
static inline uint8_t digitalPinToPort(const uint8_t P) {
if (P >= digital_pin_count) {
return NOT_A_PIN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me whether NOT_A_PIN is an invalid port number, or whether it is a valid port number that is returned for an invalid pin. Can you add a comment to make that more clear?

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you reviewing the latest commit? That should be a >=.

It's following the same convention already in place in pinMode, where the returned value of digitalPinToPort is compared against NOT_A_PIN to see if the input was a pin.

Do you think that it is not obvious from the code what the situation is, that a comment is needed?

} else {
return pgm_read_byte( digital_pin_to_port_PGM + P );
}
}
static inline uint8_t digitalPinToBitMask(const uint8_t P) {
if (P >= digital_pin_count) {
return NOT_A_PIN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here, it is not clear whether NOT_A_PIN is an invalid bit mask, or it's a valid mask that is returned for an invalid pin. Can you add a comment?

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you reviewing the latest commit? That should be a >=.

It's following the same convention already in place in pinMode, where the returned value of digitalPinToPort is compared against NOT_A_PIN to see if the input was a pin.

Do you think that it is not obvious from the code what the situation is, that a comment is needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was: the semantics of the NOT_A_PIN being used for multiple concepts that was confusing me.

The digitalPinToPort() function uses the digital_pin_to_port_PGM[] array. If an invalid pin is given, I expected an invalid value to be returned, which I expected to be named NOT_A_PORT_NUMBER. It seems like current valid port numbers (PA - PL) are > 0 so NOT_A_PORT_NUMBER can be 0. But it seems like only an accident that NOT_A_PORT_NUMBER is the same as NOT_A_PIN.

(The term "PORT" seems to be overloaded in pins_arduino.h. Sometimes it means "port number", sometimes it means a pointer. There is a NOT_A_PORT constant already defined, but it is used as a NULL or nullptr pointer as far as I can tell.)

The digitalPinToBitMask() function uses the digital_pin_to_bit_mask_PGM[] array. When an invalid pin is given, I expected a NOT_A_MASK value to be returned, a value that is not a valid bit mask, which I guess can be 0. It seems like it's an accident that a NOT_A_PIN is equal to NOT_A_MASK. Returning a NOT_A_PIN was confusing to me.

Similarly, the digitalPinToTimer() function uses the digital_pin_to_timer_PGM[] array. If an invalid pin is given, I expected an NOT_A_TIMER value to be returned, not a NOT_A_PIN. Again, it seems like an accident that NOT_A_PIN happens to be the same as NOT_A_TIMER. Now, there's already a NOT_ON_TIMER defined, and it's defined to be 0. So if you define NOT_A_TIMER to be 0, we can no longer distinguish between the 'NOT_A' and the 'NOT_ON'. Which raises the question, should `NOT_A_TIMER' be defined to be 255 instead?

These 3 functions are returning 3 conceptually separate things, and I got confused why all of them are returning a NOT_A_PIN. Anyway, if you choose not to create 3 new symbols (NOT_A_PORT_NUMBER, NOT_A_MASK, and NOT_A_TIMER), then I think some comments in the code would help explain why NOT_A_PIN can be used in all 3 cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please familiarise yourself with how these functions are currently used and implemented.They have no documentation of their API, so their existing use defines the API. wiring_digital.c and the various pins_arduino.h are good places to look. You will see that the new implementations are consistent with the existing API, as I explained above.

You are requesting that the existing API change. That is out of scope of this pull request. Please create a separate pull request or issue. I do not feel that a comment explaining that a function conforms to the rest of the (undocumented, uncommented) API is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's even more important to add comments to describe your assumptions when you are making changes on top of undocumented and uncommented API.

I have reviewed the usage of digitalPinToPort(), digitalPinToBitMask(), and digitalPinToTimer() in the following files (wiring_pulse.c, wiring_digital.c, Tone.cpp, SoftwareSerial.cpp, SPI.cpp). I support the use of NOT_A_PIN in digitalPinToPort(). But I disagree with its usage in digitalPinToBitMask() and digitalPinToTimer().

As far as I can tell, NOT_A_PIN means "an invalid port number for an invalid pin". If the symbol is overloaded to represent the concept of "an invalid mask for an invalid pin", and "an invalid timer for an invalid pin", I suspect that this will cause unnecessary confusion for (potentially hundreds of) programmers in the future who will try to deduce the meaning of NOT_A_PIN, without any explanatory comments.

If you choose to ignore my feedback and continue to use NOT_A_PIN in 3 different contexts, I request that you add comments to that part of the code that explains that it is being overloaded to mean 3 different things. I think this is a reasonable request.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For digitalPinToPort, the function conceptually returns:

  • For a valid pin with a port associated with it, the port
  • For all other cases, an error code about the pin. Exemplified by NOT_A_PIN, and the checks in wiring_digital.c

For digitalPinToTimer, the function conceptually returns:

  • For a valid pin with an associated timer, the timer associated with the pin.
  • For all other cases, an error code about the pin. Exemplified by NOT_ON_TIMER (if it were an error code about the return value, it would be as you suggested NOT_A_TIMER)

For digitalPinToBitmask, the function conceptually returns:

  • For a valid pin with an associated bitmask, the bitmask of the pin
  • For invalid pins, the return value is undefined (there are no coded cases of what it returns).
  • Given lack of an example, I think it is fair to deduce that digitalPinToBitmask should follow the same rules as the other digitalPinTo* functions.

So, for digitalPinToTimer, when passed a value that is not associated with a pin, the correct error code to return is a description of the input value. You cannot say that pin 215 is not on a timer, because pin 215 does not exist. So the correct error code is NOT_A_PIN. Given that NOT_ON_TIMER and NOT_A_PIN have the same value, existing code will work without having to check for the new possible return concept.

The same logic applies to digitalPinToBitmask. Returning zero might be acceptable too, but that would be inconsistent with the other similarly named functions.

Unfortunately it appears that the writer of port_to_input_PGM in the gemma and mega variants abuse NOT_A_PIN and use it to return when queries for the register of a port. I cannot understand what concept they are going for here, and given that portInputRegister never has its output checked, this appears to be a bug. (In most cases, when that function returns NOT_A_PIN the uC will attempt to read or write address 0x0000, which should cause a reset. At least that's better behaviour than pinMode or digitalWrite without this patch).

So:

  • if you want me to comment the return values here, then I'm solidifying what I induce about the API of these functions. Which requires fixing pins_arduino.h for the mega and gemma.
  • if I introduce new values, I'm changing the API and all dependent functions must be checked that they will still work. Plus that would just be inconsistent.

Both of those options are out of scope for this PR and more than I'm prepared to do. As I say, please deal with the issue in a separate PR.

Copy link
Contributor

@bxparks bxparks Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the intended limited scope of this PR. But your changes to digitalPinToBitMask() and digitalPinToTimer() went slightly beyond that intended scope. Those changes were not necessary for this PR, so I think my inquiry regarding those changes is legitimate.

The problem that you found in variants/gemma/pins_arduino.h and variants/mega/pins_arduino.h with their incorrect use of NOT_A_PIN is exactly the sort of problem that I want to avoid in this PR. It is obvious that those 2 files should be using NOT_A_PORT instead, to be consistent with all the others. And I agree it's not in the scope of this PR to fix those 2 files.

The return type of digitalPinToPort() is a port number. Therefore, NOT_A_PIN is port number type. It is a sentinel value that represents "an invalid port number representing an invalid pin". All current uses of NOT_A_PIN (except for variants/gemma/pins_arduino.h and variants/mega/pins_arduino.h) are consistent with it being a port number.

The return type ofdigitalPinToBitMask() is a bit mask, not a port number. I currently see no example of the digital_pin_to_bit_mask_PGM[] array using the NOT_A_PIN constant as a sentinel value to represent the concept of: "an invalid bit mask representing an invalid pin". This PR adds a new semantic meaning to NOT_A_PIN which is not appropriate, because the type of NOT_A_PIN is a port number not a bit mask. (They both happen to be a uint8_t, but that does not mean they can reuse constants. It would be like having a int getLength() function returning an INVALID_SECONDS which happens to be an int.)

Finally, the return type of digitalPinToTimer() function seems to be an integer enumeration of the various (Timer x Channel) combo. (A modern type-safe version would probably use a C++ scoped enum type (i.e. enum class). A timer value of NOT_ON_TIMER (a value of 0) is actually a valid value for this function, and it does not mean "an invalid timer value representing invalid pin". One can verify that NOT_ON_TIMER is a valid value because it is used in the analogWrite() function in cores/arduino/wiring_analog.c, where the last part of the switch statement is:

      case NOT_ON_TIMER:
      default:
        if (val < 128) {
          digitalWrite(pin, LOW);
        } else {
          digitalWrite(pin, HIGH);
        }

If digitalPinToTimer() returns a 0, the existing code continues to write into the pin, instead of interpreting the 0 to mean "invalid pin". Since NOT_A_PIN is also a 0, which overlaps with NOT_ON_TIMER, it cannot be correct for digitalPinToTimer() to return NOT_A_PIN for an invalid pin.

At this point, the simplest fix I can see that will implement the intended scope of this PR is to remove the checks for (P >= digital_pin_count) in digitalPinToBitMask() and digitalPinToTimer(), while keeping the check in digitalPinToPort(), which I agree is correct. In other words:

static inline uint8_t digitalPinToBitMask(const uint8_t P) {
   return pgm_read_byte( digital_pin_to_bit_mask_PGM + P );
}
static inline uint8_t digitalPinToTimer(const uint8_t P) {
  return pgm_read_byte( digital_pin_to_timer_PGM + P );
}

I cannot see how returning the NOT_A_PIN constant is correct for these 2 functions. I think the correct solution is to define 2 new constants, a INVALID_MASK_FOR_INVALID_PIN and a INVALID_TIMER_FOR_INVALID_PIN. But I agree it is beyond the scope of this PR to add 2 new constants, and add the appropriate checks for these sentinel values in the calling code.

} else {
return pgm_read_byte( digital_pin_to_bit_mask_PGM + P );
}
}
static inline uint8_t digitalPinToTimer(const uint8_t P) {
if (P >= digital_pin_count) {
return NOT_A_PIN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, is NOT_A_PIN an invalid Timer value, or the default value returned when the pin is not valid? Add comment?

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you reviewing the latest commit? That should be a >=.

It's following the same convention already in place in pinMode, where the returned value of digitalPinToPort is compared against NOT_A_PIN to see if the input was a pin. It doesn't return NOT_ON_TIMER because it's not a pin that's not on a timer, but it's not a pin at all. (convenient enough, by coincidence NOT_ON_TIMER and NOT_A_PIN have the same value so sloppy programming where only the former is checked will still pick it up.)

Do you think that it is not obvious from the code what the situation is, that a comment is needed?

} else {
return pgm_read_byte( digital_pin_to_timer_PGM + P );
}
}
// these defines are to maintain backwards compatibility with #ifdef
#define digitalPinToPort digitalPinToPort
#define digitalPinToBitMask digitalPinToBitMask
#define digitalPinToTimer digitalPinToTimer

#ifdef ARDUINO_MAIN
#define PA 1
Expand Down
2 changes: 2 additions & 0 deletions cores/arduino/wiring_digital.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
#include "wiring_private.h"
#include "pins_arduino.h"

const uint8_t digital_pin_count = sizeof(digital_pin_to_port_PGM) / sizeof(digital_pin_to_port_PGM[0]);
BarryPSmith marked this conversation as resolved.
Show resolved Hide resolved

void pinMode(uint8_t pin, uint8_t mode)
{
uint8_t bit = digitalPinToBitMask(pin);
Expand Down