From dc0ab33a586886e035e1c077a15bebc48829ff5e Mon Sep 17 00:00:00 2001 From: Bas Wijnen Date: Mon, 2 Jan 2023 15:23:01 +0100 Subject: [PATCH] Change definition of NO_PIN to 1+PIN_MAX (#1231) Fixes bug #1228 that gpio 0 could not be used by the linuxgpio system. * Add sanity checks * Loop over defined pins only Co-authored-by: Stefan Rueger --- src/avr.c | 2 +- src/avrftdi.c | 3 ++ src/bitbang.c | 9 ++++- src/buspirate.c | 11 +++++- src/config_gram.y | 21 ++++++---- src/libavrdude.h | 4 +- src/linuxgpio.c | 98 +++++++++++++++++++---------------------------- src/linuxspi.c | 8 ++-- src/par.c | 28 ++++++++++---- src/pgm.c | 2 +- src/pindefs.c | 4 +- src/serbb_posix.c | 11 ++++++ src/serbb_win32.c | 11 +++++- 13 files changed, 125 insertions(+), 87 deletions(-) diff --git a/src/avr.c b/src/avr.c index 732b284a..c21e63f5 100644 --- a/src/avr.c +++ b/src/avr.c @@ -769,7 +769,7 @@ int avr_write_byte_default(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM */ pgm->pgm_led(pgm, OFF); pmsg_info("this device must be powered off and back on to continue\n"); - if (pgm->pinno[PPI_AVR_VCC]) { + if ((pgm->pinno[PPI_AVR_VCC] & PIN_MASK) <= PIN_MAX) { pmsg_info("attempting to do this now ...\n"); pgm->powerdown(pgm); usleep(250000); diff --git a/src/avrftdi.c b/src/avrftdi.c index 1427743d..b255c395 100644 --- a/src/avrftdi.c +++ b/src/avrftdi.c @@ -224,6 +224,9 @@ static int set_frequency(avrftdi_t* ftdi, uint32_t freq) * here. */ static int set_pin(const PROGRAMMER *pgm, int pinfunc, int value) { + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + avrftdi_t* pdata = to_pdata(pgm); struct pindef_t pin = pgm->pin[pinfunc]; diff --git a/src/bitbang.c b/src/bitbang.c index 4d0be96e..1e017407 100644 --- a/src/bitbang.c +++ b/src/bitbang.c @@ -608,8 +608,13 @@ int bitbang_initialize(const PROGRAMMER *pgm, const AVRPART *p) { return 0; } -static int verify_pin_assigned(const PROGRAMMER *pgm, int pin, char *desc) { - if (pgm->pinno[pin] == 0) { +static int verify_pin_assigned(const PROGRAMMER *pgm, int pinfunc, char *desc) { + if(pinfunc < 0 || pinfunc >= N_PINS) { + pmsg_error("invalid pin function number %d\n", pinfunc); + return -1; + } + + if ((pgm->pinno[pinfunc] & PIN_MASK) > PIN_MAX) { pmsg_error("no pin has been assigned for %s\n", desc); return -1; } diff --git a/src/buspirate.c b/src/buspirate.c index 3df31642..94750328 100644 --- a/src/buspirate.c +++ b/src/buspirate.c @@ -1198,8 +1198,12 @@ static void buspirate_bb_enable(PROGRAMMER *pgm, const AVRPART *p) { */ static int buspirate_bb_getpin(const PROGRAMMER *pgm, int pinfunc) { unsigned char buf[10]; - int value = 0; - int pin = pgm->pinno[pinfunc]; + int pin, value = 0; + + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + + pin = pgm->pinno[pinfunc]; if (pin & PIN_INVERSE) { pin &= PIN_MASK; @@ -1261,6 +1265,9 @@ static int buspirate_bb_setpin_internal(const PROGRAMMER *pgm, int pin, int valu } static int buspirate_bb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) { + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + return buspirate_bb_setpin_internal(pgm, pgm->pinno[pinfunc], value); } diff --git a/src/config_gram.y b/src/config_gram.y index 3052eaa3..257abf61 100644 --- a/src/config_gram.y +++ b/src/config_gram.y @@ -44,7 +44,7 @@ int yylex(void); int yyerror(char * errmsg, ...); int yywarning(char * errmsg, ...); -static int assign_pin(int pinno, TOKEN * v, int invert); +static int assign_pin(int pinfunc, TOKEN *v, int invert); static int assign_pin_list(int invert); static int which_opcode(TOKEN * opcode); static int parse_cmdbits(OPCODE * op, int opnum); @@ -1550,11 +1550,13 @@ static char * vtypestr(int type) #endif -static int assign_pin(int pinno, TOKEN * v, int invert) -{ - int value; +static int assign_pin(int pinfunc, TOKEN *v, int invert) { + if(pinfunc < 0 || pinfunc >= N_PINS) { + yyerror("pin function must be in the range [0, %d]", N_PINS-1); + return -1; + } - value = v->value.number; + int value = v->value.number; free_token(v); if ((value < PIN_MIN) || (value > PIN_MAX)) { @@ -1562,7 +1564,7 @@ static int assign_pin(int pinno, TOKEN * v, int invert) return -1; } - pin_set_value(&(current_prog->pin[pinno]), value, invert); + pin_set_value(&(current_prog->pin[pinfunc]), value, invert); return 0; } @@ -1573,7 +1575,12 @@ static int assign_pin_list(int invert) int pin; int rv = 0; - current_prog->pinno[pin_name] = 0; + if(pin_name < 0 || pin_name >= N_PINS) { + yyerror("pin_name should be in the range [0, %d]", N_PINS-1); + return -1; + } + + current_prog->pinno[pin_name] = NO_PIN; while (lsize(number_list)) { t = lrmv_n(number_list, 1); if (rv == 0) { diff --git a/src/libavrdude.h b/src/libavrdude.h index a86092ce..70347037 100644 --- a/src/libavrdude.h +++ b/src/libavrdude.h @@ -434,7 +434,7 @@ enum { /** Number of pins in each element of the bitfield */ #define PIN_FIELD_ELEMENT_SIZE (sizeof(pinmask_t) * 8) /** Numer of elements to store the complete bitfield of all pins */ -#define PIN_FIELD_SIZE ((PIN_MAX + PIN_FIELD_ELEMENT_SIZE)/PIN_FIELD_ELEMENT_SIZE) +#define PIN_FIELD_SIZE ((PIN_MAX+1 + PIN_FIELD_ELEMENT_SIZE-1)/PIN_FIELD_ELEMENT_SIZE) /** * This sets the corresponding bits to 1 or 0, the inverse mask is used to invert the value in necessary. @@ -832,6 +832,8 @@ typedef struct programmer_t { char flag; // For use by pgm->initpgm() } PROGRAMMER; +#define NO_PIN (PIN_MAX + 1U) // Magic pinno[] value for unused pins + #ifdef __cplusplus extern "C" { #endif diff --git a/src/linuxgpio.c b/src/linuxgpio.c index 5f98b938..cf6c3278 100644 --- a/src/linuxgpio.c +++ b/src/linuxgpio.c @@ -47,10 +47,8 @@ /* * GPIO user space helpers - * The following functions are acting on an "unsigned gpio" argument, which corresponds to the - * gpio numbering scheme in the kernel (starting from 0). - * The higher level functions use "int pin" to specify the pins with an offset of 1: - * gpio = pin - 1; + * The following functions are acting on an "unsigned gpio" argument, which corresponds to the + * gpio numbering scheme in the kernel (starting from 0). */ #define GPIO_DIR_IN 0 @@ -151,24 +149,19 @@ static int linuxgpio_fds[N_GPIO] ; static int linuxgpio_setpin(const PROGRAMMER *pgm, int pinfunc, int value) { - int r; - int pin = pgm->pinno[pinfunc]; // TODO - - if (pin & PIN_INVERSE) - { - value = !value; - pin &= PIN_MASK; - } - - if ( linuxgpio_fds[pin] < 0 ) + if(pinfunc < 0 || pinfunc >= N_PINS) return -1; - if (value) - r = write(linuxgpio_fds[pin], "1", 1); - else - r = write(linuxgpio_fds[pin], "0", 1); + unsigned pin = pgm->pinno[pinfunc]; + if (pin & PIN_INVERSE) + value = !value; + pin &= PIN_MASK; - if (r!=1) return -1; + if (pin > PIN_MAX || linuxgpio_fds[pin] < 0) + return -1; + + if (write(linuxgpio_fds[pin], value? "1": "0", 1) != 1) + return -1; if (pgm->ispdelay > 1) bitbang_delay(pgm->ispdelay); @@ -177,37 +170,33 @@ static int linuxgpio_setpin(const PROGRAMMER *pgm, int pinfunc, int value) { } static int linuxgpio_getpin(const PROGRAMMER *pgm, int pinfunc) { - unsigned char invert=0; + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + + unsigned int pin = pgm->pinno[pinfunc]; + int invert = !!(pin & PIN_INVERSE); + pin &= PIN_MASK; + + if(pin > PIN_MAX || linuxgpio_fds[pin] < 0) + return -1; + + if(lseek(linuxgpio_fds[pin], 0, SEEK_SET) < 0) + return -1; + char c; - int pin = pgm->pinno[pinfunc]; // TODO - - if (pin & PIN_INVERSE) - { - invert = 1; - pin &= PIN_MASK; - } - - if ( linuxgpio_fds[pin] < 0 ) + if(read(linuxgpio_fds[pin], &c, 1) != 1) return -1; - if (lseek(linuxgpio_fds[pin], 0, SEEK_SET)<0) - return -1; - - if (read(linuxgpio_fds[pin], &c, 1)!=1) - return -1; - - if (c=='0') - return 0+invert; - else if (c=='1') - return 1-invert; - else - return -1; + return c=='0'? 0+invert: c=='1'? 1-invert: -1; } static int linuxgpio_highpulsepin(const PROGRAMMER *pgm, int pinfunc) { - int pin = pgm->pinno[pinfunc]; // TODO - - if ( linuxgpio_fds[pin & PIN_MASK] < 0 ) + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + + unsigned int pin = pgm->pinno[pinfunc] & PIN_MASK; + + if (pin > PIN_MAX || linuxgpio_fds[pin] < 0 ) return -1; linuxgpio_setpin(pgm, pinfunc, 1); @@ -250,20 +239,9 @@ static int linuxgpio_open(PROGRAMMER *pgm, const char *port) { for (i=0; ipinno[i] & PIN_MASK) != 0 || - i == PIN_AVR_RESET || - i == PIN_AVR_SCK || - i == PIN_AVR_SDO || - i == PIN_AVR_SDI ) { + // Avrdude assumes that if a pin number is invalid it means not used/available + for (i = 1; i < N_PINS; i++) { // The pin enumeration in libavrdude.h starts with PPI_AVR_VCC = 1 + if ((pgm->pinno[i] & PIN_MASK) <= PIN_MAX) { pin = pgm->pinno[i] & PIN_MASK; if ((r=linuxgpio_export(pin)) < 0) { pmsg_ext_error("cannot export GPIO %d, already exported/busy?: %s", @@ -333,13 +311,15 @@ static void linuxgpio_close(PROGRAMMER *pgm) for (i=0; i= 0 && i != reset_pin) { close(linuxgpio_fds[i]); + linuxgpio_fds[i] = -1; linuxgpio_dir_in(i); linuxgpio_unexport(i); } } //configure RESET as input, if there's external pull up it will go high - if (linuxgpio_fds[reset_pin] >= 0) { + if(reset_pin <= PIN_MAX && linuxgpio_fds[reset_pin] >= 0) { close(linuxgpio_fds[reset_pin]); + linuxgpio_fds[reset_pin] = -1; linuxgpio_dir_in(reset_pin); linuxgpio_unexport(reset_pin); } diff --git a/src/linuxspi.c b/src/linuxspi.c index e45ca687..bc4a3788 100644 --- a/src/linuxspi.c +++ b/src/linuxspi.c @@ -134,7 +134,7 @@ static int linuxspi_reset_mcu(const PROGRAMMER *pgm, bool active) { #endif if (ret == -1) { ret = -errno; - pmsg_ext_error("unable to set GPIO line %d value: %s\n", pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE, strerror(errno)); + pmsg_ext_error("unable to set GPIO line %d value: %s\n", pgm->pinno[PIN_AVR_RESET] & PIN_MASK, strerror(errno)); return ret; } @@ -200,7 +200,7 @@ static int linuxspi_open(PROGRAMMER *pgm, const char *pt) { strcpy(req.consumer_label, progname); req.lines = 1; - req.lineoffsets[0] = pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE; + req.lineoffsets[0] = pgm->pinno[PIN_AVR_RESET] & PIN_MASK; req.default_values[0] = !!(pgm->pinno[PIN_AVR_RESET] & PIN_INVERSE); req.flags = GPIOHANDLE_REQUEST_OUTPUT; @@ -212,7 +212,7 @@ static int linuxspi_open(PROGRAMMER *pgm, const char *pt) { struct gpio_v2_line_request reqv2; memset(&reqv2, 0, sizeof(reqv2)); - reqv2.offsets[0] = pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE; + reqv2.offsets[0] = pgm->pinno[PIN_AVR_RESET] & PIN_MASK; strncpy(reqv2.consumer, progname, sizeof(reqv2.consumer) - 1); reqv2.config.flags = GPIO_V2_LINE_FLAG_OUTPUT; reqv2.config.num_attrs = 1; @@ -228,7 +228,7 @@ static int linuxspi_open(PROGRAMMER *pgm, const char *pt) { #endif if (ret == -1) { ret = -errno; - pmsg_ext_error("unable to get GPIO line %d. %s\n", pgm->pinno[PIN_AVR_RESET] & ~PIN_INVERSE, strerror(errno)); + pmsg_ext_error("unable to get GPIO line %d. %s\n", pgm->pinno[PIN_AVR_RESET] & PIN_MASK, strerror(errno)); goto close_gpiochip; } diff --git a/src/par.c b/src/par.c index 3b594302..da0e90cd 100644 --- a/src/par.c +++ b/src/par.c @@ -102,12 +102,19 @@ static int par_setpin_internal(const PROGRAMMER *pgm, int pin, int value) { } static int par_setpin(const PROGRAMMER * pgm, int pinfunc, int value) { + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + return par_setpin_internal(pgm, pgm->pinno[pinfunc], value); } static void par_setmany(const PROGRAMMER *pgm, int pinfunc, int value) { - int pin, mask; - int pinset = pgm->pinno[pinfunc]; + int pin, mask, pinset; + + if(pinfunc < 0 || pinfunc >= N_PINS) + return; + + pinset = pgm->pinno[pinfunc]; /* mask is anything non-pin - needs to be applied to each par_setpin to preserve inversion */ mask = pinset & (~PIN_MASK); @@ -119,9 +126,12 @@ static void par_setmany(const PROGRAMMER *pgm, int pinfunc, int value) { } static int par_getpin(const PROGRAMMER * pgm, int pinfunc) { - int value; - int inverted; - int pin = pgm->pinno[pinfunc]; + int value, inverted, pin; + + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + + pin = pgm->pinno[pinfunc]; inverted = pin & PIN_INVERSE; pin &= PIN_MASK; @@ -147,8 +157,12 @@ static int par_getpin(const PROGRAMMER * pgm, int pinfunc) { static int par_highpulsepin(const PROGRAMMER *pgm, int pinfunc) { - int inverted; - int pin = pgm->pinno[pinfunc]; + int inverted, pin; + + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + + pin = pgm->pinno[pinfunc]; inverted = pin & PIN_INVERSE; pin &= PIN_MASK; diff --git a/src/pgm.c b/src/pgm.c index 34006d53..d298aa90 100644 --- a/src/pgm.c +++ b/src/pgm.c @@ -89,7 +89,7 @@ PROGRAMMER *pgm_new(void) { // Clear pin array for(int i=0; ipinno[i] = 0; + pgm->pinno[i] = NO_PIN; pin_clear_all(&(pgm->pin[i])); } diff --git a/src/pindefs.c b/src/pindefs.c index db127992..f28c5fde 100644 --- a/src/pindefs.c +++ b/src/pindefs.c @@ -61,7 +61,7 @@ void pin_clear_all(struct pindef_t * const pindef) { static int pin_fill_old_pinno(const struct pindef_t * const pindef, unsigned int * const pinno) { bool found = false; int i; - for(i = 0; i < PIN_MAX; i++) { + for(i = 0; i <= PIN_MAX; i++) { if(pindef->mask[i / PIN_FIELD_ELEMENT_SIZE] & (1 << (i % PIN_FIELD_ELEMENT_SIZE))) { if(found) { pmsg_error("multiple pins found\n"); // TODO @@ -94,7 +94,7 @@ static int pin_fill_old_pinlist(const struct pindef_t * const pindef, unsigned i } if (pindef->mask[i] == 0) { /* this pin function is not using any pins */ - *pinno = 0; + *pinno = NO_PIN; } else if(pindef->mask[i] == pindef->inverse[i]) { /* all set bits in mask are set in inverse */ *pinno = pindef->mask[i]; *pinno |= PIN_INVERSE; diff --git a/src/serbb_posix.c b/src/serbb_posix.c index 0a870335..9458509b 100644 --- a/src/serbb_posix.c +++ b/src/serbb_posix.c @@ -73,6 +73,10 @@ static char *serpins[DB9PINS + 1] = static int serbb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) { unsigned int ctl; int r; + + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + int pin = pgm->pinno[pinfunc]; // get its value if (pin & PIN_INVERSE) @@ -130,6 +134,10 @@ static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) { unsigned int ctl; unsigned char invert; int r; + + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + int pin = pgm->pinno[pinfunc]; // get its value if (pin & PIN_INVERSE) @@ -177,6 +185,9 @@ static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) { } static int serbb_highpulsepin(const PROGRAMMER *pgm, int pinfunc) { + if (pinfunc < 0 || pinfunc >= N_PINS) + return -1; + int pin = pgm->pinno[pinfunc]; // replace pin name by its value if ( (pin & PIN_MASK) < 1 || (pin & PIN_MASK) > DB9PINS ) diff --git a/src/serbb_win32.c b/src/serbb_win32.c index 6a718852..fa41d04c 100644 --- a/src/serbb_win32.c +++ b/src/serbb_win32.c @@ -62,6 +62,9 @@ static int dtr, rts, txd; #define DB9PINS 9 static int serbb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) { + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + int pin = pgm->pinno[pinfunc]; HANDLE hComPort = (HANDLE)pgm->fd.pfd; LPVOID lpMsgBuf; @@ -126,6 +129,9 @@ static int serbb_setpin(const PROGRAMMER *pgm, int pinfunc, int value) { } static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) { + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + int pin = pgm->pinno[pinfunc]; HANDLE hComPort = (HANDLE)pgm->fd.pfd; LPVOID lpMsgBuf; @@ -208,7 +214,10 @@ static int serbb_getpin(const PROGRAMMER *pgm, int pinfunc) { } static int serbb_highpulsepin(const PROGRAMMER *pgm, int pinfunc) { - int pin = pgm->pinno[pinfunc]; + if(pinfunc < 0 || pinfunc >= N_PINS) + return -1; + + int pin = pgm->pinno[pinfunc]; if ( (pin & PIN_MASK) < 1 || (pin & PIN_MASK) > DB9PINS ) return -1;