coverity/13683: Out-of-bounds access: RI.

Problem    : Out-of-bounds access @ 3730.
Diagnostic : Real issue.
Rationale  : str is constructed step by step, str_l growing each time.
             str_m is the maximum length of str. So, at every step,
             avail is computed to see if the piece to be added fits in.
             If not, piece is truncated to a max of `avail`, so that str
             stays in bounds. Such blocks where pieces are added are of
             the form `if (str_l < str_m)`. It then follows that once
             one of those pieces exhausts available space on str, no
             other such block should be entered. Formally:

               str_l < strl_m && avail = str_m - str_l && x >= avail
                                    -->
                             str_l + x >= str_m

             Now, suggested error path successively enters blocks where
             str is exhausted. We're not sure if coverity just fails to
             follow above implications, or, on the contrary, it's aware
             of them, but it's signaling the more complex possibility of
             implications not being fulfilled because of possible
             arithmetic overflows. We opt then to assume this last case,
             as the possibility is in fact there.
Resolution : Refactor code so that tracked condition doesn't depend on
             arithmetic implications. Check for overflow.
This commit is contained in:
Eliseo Martínez 2015-02-14 00:03:19 +01:00
parent 33cecbbf16
commit 24fa25a57f

View File

@ -3101,6 +3101,7 @@ int vim_snprintf(char *str, size_t str_m, char *fmt, ...)
int vim_vsnprintf(char *str, size_t str_m, char *fmt, va_list ap, typval_T *tvs) int vim_vsnprintf(char *str, size_t str_m, char *fmt, va_list ap, typval_T *tvs)
{ {
size_t str_l = 0; size_t str_l = 0;
bool str_avail = str_l < str_m;
char *p = fmt; char *p = fmt;
int arg_idx = 1; int arg_idx = 1;
@ -3108,15 +3109,15 @@ int vim_vsnprintf(char *str, size_t str_m, char *fmt, va_list ap, typval_T *tvs)
p = ""; p = "";
while (*p != NUL) { while (*p != NUL) {
if (*p != '%') { if (*p != '%') {
size_t n = xstrchrnul(p + 1, '%') - p;
/* Copy up to the next '%' or NUL without any changes. */ /* Copy up to the next '%' or NUL without any changes. */
if (str_l < str_m) { size_t n = (size_t)(xstrchrnul(p + 1, '%') - p);
if (str_avail) {
size_t avail = str_m - str_l; size_t avail = str_m - str_l;
memmove(str + str_l, p, MIN(n, avail));
memmove(str + str_l, p, n > avail ? avail : n); str_avail = n < avail;
} }
p += n; p += n;
assert(n <= SIZE_MAX - str_l);
str_l += n; str_l += n;
} else { } else {
size_t min_field_width = 0, precision = 0; size_t min_field_width = 0, precision = 0;
@ -3666,17 +3667,16 @@ int vim_vsnprintf(char *str, size_t str_m, char *fmt, va_list ap, typval_T *tvs)
* this does not include the zero padding in case of numerical * this does not include the zero padding in case of numerical
* conversions*/ * conversions*/
if (!justify_left) { if (!justify_left) {
/* left padding with blank or zero */ assert(str_arg_l <= SIZE_MAX - number_of_zeros_to_pad);
int pn = (int)(min_field_width - (str_arg_l + number_of_zeros_to_pad)); if (min_field_width > str_arg_l + number_of_zeros_to_pad) {
/* left padding with blank or zero */
if (pn > 0) { size_t pn = min_field_width - (str_arg_l + number_of_zeros_to_pad);
if (str_l < str_m) { if (str_avail) {
size_t avail = str_m - str_l; size_t avail = str_m - str_l;
memset(str + str_l, zero_padding ? '0' : ' ', MIN(pn, avail));
memset(str + str_l, zero_padding ? '0' : ' ', str_avail = pn < avail;
(size_t)pn > avail ? avail
: (size_t)pn);
} }
assert(pn <= SIZE_MAX - str_l);
str_l += pn; str_l += pn;
} }
} }
@ -3688,67 +3688,58 @@ int vim_vsnprintf(char *str, size_t str_m, char *fmt, va_list ap, typval_T *tvs)
* force it to be copied later in its entirety */ * force it to be copied later in its entirety */
zero_padding_insertion_ind = 0; zero_padding_insertion_ind = 0;
} else { } else {
/* insert first part of numerics (sign or '0x') before zero /* insert first part of numerics (sign or '0x') before zero padding */
* padding */ if (zero_padding_insertion_ind > 0) {
int zn = (int)zero_padding_insertion_ind; size_t zn = zero_padding_insertion_ind;
if (str_avail) {
if (zn > 0) {
if (str_l < str_m) {
size_t avail = str_m - str_l; size_t avail = str_m - str_l;
memmove(str + str_l, str_arg, MIN(zn, avail));
memmove(str + str_l, str_arg, str_avail = zn < avail;
(size_t)zn > avail ? avail
: (size_t)zn);
} }
assert(zn <= SIZE_MAX - str_l);
str_l += zn; str_l += zn;
} }
/* insert zero padding as requested by the precision or min /* insert zero padding as requested by precision or min field width */
* field width */ if (number_of_zeros_to_pad > 0) {
zn = (int)number_of_zeros_to_pad; size_t zn = number_of_zeros_to_pad;
if (zn > 0) { if (str_avail) {
if (str_l < str_m) { size_t avail = str_m - str_l;
size_t avail = str_m-str_l; memset(str + str_l, '0', MIN(zn, avail));
str_avail = zn < avail;
memset(str + str_l, '0',
(size_t)zn > avail ? avail
: (size_t)zn);
} }
assert(zn <= SIZE_MAX - str_l);
str_l += zn; str_l += zn;
} }
} }
/* insert formatted string /* insert formatted string
* (or as-is conversion specifier for unknown conversions) */ * (or as-is conversion specifier for unknown conversions) */
{ if (str_arg_l > zero_padding_insertion_ind) {
int sn = (int)(str_arg_l - zero_padding_insertion_ind); size_t sn = str_arg_l - zero_padding_insertion_ind;
if (str_avail) {
if (sn > 0) { size_t avail = str_m - str_l;
if (str_l < str_m) { memmove(str + str_l,
size_t avail = str_m - str_l; str_arg + zero_padding_insertion_ind,
MIN(sn, avail));
memmove(str + str_l, str_avail = sn < avail;
str_arg + zero_padding_insertion_ind,
(size_t)sn > avail ? avail : (size_t)sn);
}
str_l += sn;
} }
assert(sn <= SIZE_MAX - str_l);
str_l += sn;
} }
/* insert right padding */ /* insert right padding */
if (justify_left) { if (justify_left) {
/* right blank padding to the field width */ assert(str_arg_l <= SIZE_MAX - number_of_zeros_to_pad);
int pn = (int)(min_field_width if (min_field_width > str_arg_l + number_of_zeros_to_pad) {
- (str_arg_l + number_of_zeros_to_pad)); /* right blank padding to the field width */
size_t pn = min_field_width - (str_arg_l + number_of_zeros_to_pad);
if (pn > 0) { if (str_avail) {
if (str_l < str_m) {
size_t avail = str_m - str_l; size_t avail = str_m - str_l;
memset(str + str_l, ' ', MIN(pn, avail));
memset(str + str_l, ' ', str_avail = pn < avail;
(size_t)pn > avail ? avail
: (size_t)pn);
} }
assert(pn <= SIZE_MAX - str_l);
str_l += pn; str_l += pn;
} }
} }