The problem scenario is that a user enters a string with a number that has invalid data or is too large for uint8_t. The result is that the current code would then use LONG_MAX or LONG_MIN (truncated to one byte). Not a big problem, but an error message would be better.
Comments on the routine below: - No NULL pointer checks on the 3 input parameters. - Is portability to Windows a concern? If so, a wide-char sub-case might need to be recognized. Or at least tested to make sure it returns an error. - Either do not print messages inside the routine, but return an error code and let the caller do that. OR, if you are going to print the messages inside the routine, why not do it all in the routine? Pass in a min & max value for variable-specify bounds checking. My $.02 Andy -----Original Message----- From: Zdenek Styblik [mailto:zdenek.styb...@gmail.com] Sent: Monday, October 24, 2011 2:25 PM To: ipmitool-devel Subject: [Ipmitool-devel] multiple uint8_t overflow in 'lib/ipmi_main.c' viaparameters Hello, as of now, it is possible to cause uint8_t overflows via parameters to ipmitool. Example code to blame from 'lib/ipmi_main.c': ~~~ SNIP ~~~ case 't': target_addr = (uint8_t)strtol(optarg, NULL, 0); break; ~~~ SNIP ~~~ No check is being made whether only numerical input has been given nor whether long resp. unsigned long resp. unsigned char overflew. Proposed solution is to create generic function/functions in 'lib/helper.c' to handle this issue and save some lines by not repeating the code. ~~~ 'lib/helper.c' ~~~ #include <limits.h> [...] /* Desc: Convert array of chars into uint8_t and check for overflows * @str: array of chars to parse from * @uchr: pointer to address where uint8_t will be stored * @label: label to print on/in error message. */ int str2uchar(char *str, uint8_t *uchr, char *label) { uint32_t arg_long = 0; char *end_ptr = 0; if (sizeof(str) <= 0 || !arg) { return (-1); } errno = 0; arg_long = strtoul(str, &end_ptr, 0); if (*end_ptr != '\0' || errno != 0 || arg_long < 0) { /* invalid input/overflow */ lprintf(LOG_ERR, "'%s': Invalid input given.\n", label); return (-1); } if (arg_long > UCHAR_MAX || arg_long == LONG_MIN || arg_long == LONG_MAX) { /* arg is too big to fit uint8_t */ lprintf(LOG_ERR, "'%s': Input is out of range.\n", label); return (-1); } *uchr = (uint8_t) arg_long; return 0; } ~~~ 'lib/helper.c' ~~~ And then its utilization: ~~~ 'lib/ipmi_main.c' ~~~ case 'R': if (str2uchar(optarg, &retry, "-R") != 0) { goto out_free; } ~~~ 'lib/ipmi_main.c' ~~~ I'm sorry for not having diffs now. I'll be grateful ... no, I "demand" code review, comments, tips for variable names, whatever comes to your mind. Have a nice day, Zdenek ------------------------------------------------------------------------ ------ The demand for IT networking professionals continues to grow, and the demand for specialized networking skills is growing even more rapidly. Take a complimentary Learning@Cisco Self-Assessment and learn about Cisco certifications, training, and career opportunities. http://p.sf.net/sfu/cisco-dev2dev _______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel ------------------------------------------------------------------------------ The demand for IT networking professionals continues to grow, and the demand for specialized networking skills is growing even more rapidly. Take a complimentary Learning@Cisco Self-Assessment and learn about Cisco certifications, training, and career opportunities. http://p.sf.net/sfu/cisco-dev2dev _______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
