Thank you for your comments.
On Mon, Oct 24, 2011 at 9:11 PM, Andy Cress <andy.cr...@us.kontron.com> wrote:
>
> 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.
>
Hm.
> Comments on the routine below:
> - No NULL pointer checks on the 3 input parameters.
I presume this:
~~~
if (!str || !uchr || !label || sizeof(str) <= 0 || sizeof(label) <= 0) {
~~~
should handle it, right?
> - 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.
Why not. Anyway, I have no experience with Windows nor I really want to,
but that doesn't mean I'm not interested. Could you elaborate a bit more on
this one? Quick search yielded nothing useful resp. I assume wchar_t is not
what you had in mind.
> - 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.
>
I gave it a thought and you are right. It is going to be better if
errors resp. printing
is going to be handled by caller. Otherwise it would defy general
purpose function
which was a bit of aim.
So, RC 1 for invalid params, RC 2 for overflow/invalid input, RC 3 when input
fits long, yet is out of range of uchar?
Z.
> 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
