Project

General

Profile

Bug #633

utils_parse_size_suffix suffers from several problems

Added by Daniel U. Thibault over 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Target version:
Start date:
09/18/2013
Due date:
% Done:

100%

Estimated time:

Description

The lttng-tools/src/common/utils.c utils_parse_size_suffix function is used by the snapshot command to handle its max-size option and the enable-channel command to handle its subbuf-size and tracefile-size options. As currently written, it suffers from several problems.

  • It accepts a leading 0 (without a following x or X) to represent an octal value, but simultaneously accepts the digits 8 and 9 in said octal value (this yields –1).
  • It does not support the leading 0X hexadecimal indicator but does support a leading 0x (see following).
  • It accepts a leading 0x to represent an hexadecimal value but rejects the a..f and A..F digits in said value.

The function uses strtoull(); for that function's string argument specification, see strtol() at http://www.cplusplus.com/reference/cstdlib/strtol/

int utils_parse_size_suffix(char *str, uint64_t *size)
{
[...]
    /* Compile regex */
    ret = regcomp(&regex, "^\\(0x\\)\\{0,1\\}[0-9][0-9]*\\([kKMG]\\{0,1\\}\\)$", 0);
[...]
    base_size = strtoull(str, NULL, 0);
    if (errno != 0) {
[...]
}

I think the regex should rather be:

    ret = regcomp(&regex, "^(((0x|0X)[0-9A-Fa-f]+)|(0[0-7]*)|([1-9][0-9]*))[kKMG]?$", REG_EXTENDED);

(Someone needs to check my syntax) (thanks to http://www.greenend.org.uk/rjk/tech/regexp.html)

Alternately, the range of strings utils_parse_size_suffix accepts could be restricted (to drop octal support, for instance).

#1

Updated by Simon Marchi over 6 years ago

Good points. Your regex is very easy to understand (hex|oct|dec). I suggest you submit a patch and augment the test cases in tests/unit/test_utils_parse_size_suffix.c which are clearly lacking, since they didn't catch these bugs.

#2

Updated by David Goulet over 6 years ago

  • Status changed from New to Confirmed
#3

Updated by David Goulet about 6 years ago

  • Status changed from Confirmed to Feedback

Is it possible for you to submit a patch with that fixes and also, like Simon suggested, improve the unit test?

#4

Updated by Simon Marchi almost 6 years ago

  • Status changed from Feedback to Resolved
  • % Done changed from 0 to 100

Also available in: Atom PDF