| Message ID | 20210127085752.120571-3-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with -Wextra | expand |
On 27/01/2021 09.57, Alexey Kardashevskiy wrote: > -Wextra enables a bunch of rather useful checks which this fixes. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > lib/libc/stdio/vsnprintf.c | 15 ++++++++------- > lib/libc/string/memmove.c | 2 +- > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c > index 21dd04dfe46f..1a44301f74da 100644 > --- a/lib/libc/stdio/vsnprintf.c > +++ b/lib/libc/stdio/vsnprintf.c > @@ -25,15 +25,15 @@ static int > print_str_fill(char **buffer, size_t bufsize, char *sizec, > const char *str, char c) > { > - int i, sizei, len; > + unsigned i, sizei, len; > char *bstart = *buffer; > > sizei = strtoul(sizec, NULL, 10); > len = strlen(str); > if (sizei > len) { > for (i = 0; > - (i < (sizei - len)) && ((*buffer - bstart) < bufsize); > - i++) { > + (i < (sizei - len)) && ((*buffer - bstart) < (int)bufsize); bufsize is of type size_t, so the correct way to cast would be "(ssize_t)" instead of "(int)", I think. Maybe it would also be worth the effort to introduce a new local variable a la: ssize_t sbufsize = (ssize_t)bufsize; so that you don't have to do the cast again and again all over the place? Thomas > + i++) { > **buffer = c; > *buffer += 1; > } > @@ -47,7 +47,7 @@ print_str(char **buffer, size_t bufsize, const char *str) > char *bstart = *buffer; > size_t i; > > - for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < bufsize); i++) { > + for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < (int)bufsize); i++) { > **buffer = str[i]; > *buffer += 1; > } > @@ -112,7 +112,7 @@ print_fill(char **buffer, size_t bufsize, char *sizec, unsigned long size, > len = print_intlen(size, base) + optlen; > if (sizei > len) { > for (i = 0; > - (i < (sizei - len)) && ((*buffer - bstart) < bufsize); > + (i < (sizei - len)) && ((*buffer - bstart) < (int)bufsize); > i++) { > **buffer = c; > *buffer += 1; > @@ -143,7 +143,7 @@ print_format(char **buffer, size_t bufsize, const char *format, void *var) > form++; > } > > - while ((*form != '\0') && ((*buffer - start) < bufsize)) { > + while ((*form != '\0') && ((*buffer - start) < (int)bufsize)) { > switch(*form) { > case 'u': > case 'd': > @@ -163,6 +163,7 @@ print_format(char **buffer, size_t bufsize, const char *format, void *var) > break; > case 'X': > upper = true; > + /* fallthrough */ > case 'x': > sizec[i] = '\0'; > value = (unsigned long) var & convert[length_mod]; > @@ -260,7 +261,7 @@ vsnprintf(char *buffer, size_t bufsize, const char *format, va_list arg) > /* Leave one space for NULL character */ > bufsize--; > > - while(*ptr != '\0' && (buffer - bstart) < bufsize) > + while(*ptr != '\0' && (buffer - bstart) < (int)bufsize) > { > if(*ptr == '%') { > char formstr[20]; > diff --git a/lib/libc/string/memmove.c b/lib/libc/string/memmove.c > index 3acf1a973bbe..9d0962847296 100644 > --- a/lib/libc/string/memmove.c > +++ b/lib/libc/string/memmove.c > @@ -18,7 +18,7 @@ memmove(void *dest, const void *src, size_t n) > { > char *cdest; > const char *csrc; > - int i; > + size_t i; > > /* Do the buffers overlap in a bad way? */ > if (src < dest && src + n >= dest) { >
On Wed, Jan 27, 2021 at 01:58:26PM +0100, Thomas Huth wrote: > On 27/01/2021 09.57, Alexey Kardashevskiy wrote: > >- (i < (sizei - len)) && ((*buffer - bstart) < > >bufsize); > >- i++) > >{ > >+ (i < (sizei - len)) && ((*buffer - bstart) < > >(int)bufsize); > > bufsize is of type size_t, so the correct way to cast would be "(ssize_t)" > instead of "(int)", I think. > Maybe it would also be worth the effort to introduce a new local variable a > la: > > ssize_t sbufsize = (ssize_t)bufsize; > > so that you don't have to do the cast again and again all over the place? Or, better: i < (sizei - len) && (size_t)(*buffer - bstart) < bufsize If the difference between the two pointers is negative (which it never should be, but just assume it is), the cast to an unsigned type will make the comparison return false, exactly as we want. Segher
On 28/01/2021 09:48, Segher Boessenkool wrote: > On Wed, Jan 27, 2021 at 01:58:26PM +0100, Thomas Huth wrote: >> On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >>> - (i < (sizei - len)) && ((*buffer - bstart) < >>> bufsize); >>> - i++) >>> { >>> + (i < (sizei - len)) && ((*buffer - bstart) < >>> (int)bufsize); >> >> bufsize is of type size_t, so the correct way to cast would be "(ssize_t)" >> instead of "(int)", I think. >> Maybe it would also be worth the effort to introduce a new local variable a >> la: >> >> ssize_t sbufsize = (ssize_t)bufsize; >> >> so that you don't have to do the cast again and again all over the place? > > Or, better: > i < (sizei - len) && (size_t)(*buffer - bstart) < bufsize > > If the difference between the two pointers is negative (which it never > should be, but just assume it is), the cast to an unsigned type will > make the comparison return false, exactly as we want. ah this is neat, I did not think about it this way :)
On 27/01/2021 23:58, Thomas Huth wrote: > On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >> -Wextra enables a bunch of rather useful checks which this fixes. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> lib/libc/stdio/vsnprintf.c | 15 ++++++++------- >> lib/libc/string/memmove.c | 2 +- >> 2 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c >> index 21dd04dfe46f..1a44301f74da 100644 >> --- a/lib/libc/stdio/vsnprintf.c >> +++ b/lib/libc/stdio/vsnprintf.c >> @@ -25,15 +25,15 @@ static int >> print_str_fill(char **buffer, size_t bufsize, char *sizec, >> const char *str, char c) >> { >> - int i, sizei, len; >> + unsigned i, sizei, len; >> char *bstart = *buffer; >> sizei = strtoul(sizec, NULL, 10); >> len = strlen(str); >> if (sizei > len) { >> for (i = 0; >> - (i < (sizei - len)) && ((*buffer - bstart) < bufsize); >> - i++) { >> + (i < (sizei - len)) && ((*buffer - bstart) < (int)bufsize); > > bufsize is of type size_t, so the correct way to cast would be > "(ssize_t)" instead of "(int)", I think. > Maybe it would also be worth the effort to introduce a new local > variable a la: > > ssize_t sbufsize = (ssize_t)bufsize; > > so that you don't have to do the cast again and again all over the place? This is done once per a function, does not seem worth the variable, unless I missed something? Thanks, > > Thomas > > >> + i++) { >> **buffer = c; >> *buffer += 1; >> } >> @@ -47,7 +47,7 @@ print_str(char **buffer, size_t bufsize, const char >> *str) >> char *bstart = *buffer; >> size_t i; >> - for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < bufsize); >> i++) { >> + for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < >> (int)bufsize); i++) { >> **buffer = str[i]; >> *buffer += 1; >> } >> @@ -112,7 +112,7 @@ print_fill(char **buffer, size_t bufsize, char >> *sizec, unsigned long size, >> len = print_intlen(size, base) + optlen; >> if (sizei > len) { >> for (i = 0; >> - (i < (sizei - len)) && ((*buffer - bstart) < bufsize); >> + (i < (sizei - len)) && ((*buffer - bstart) < (int)bufsize); >> i++) { >> **buffer = c; >> *buffer += 1; >> @@ -143,7 +143,7 @@ print_format(char **buffer, size_t bufsize, const >> char *format, void *var) >> form++; >> } >> - while ((*form != '\0') && ((*buffer - start) < bufsize)) { >> + while ((*form != '\0') && ((*buffer - start) < (int)bufsize)) { >> switch(*form) { >> case 'u': >> case 'd': >> @@ -163,6 +163,7 @@ print_format(char **buffer, size_t bufsize, const >> char *format, void *var) >> break; >> case 'X': >> upper = true; >> + /* fallthrough */ >> case 'x': >> sizec[i] = '\0'; >> value = (unsigned long) var & convert[length_mod]; >> @@ -260,7 +261,7 @@ vsnprintf(char *buffer, size_t bufsize, const char >> *format, va_list arg) >> /* Leave one space for NULL character */ >> bufsize--; >> - while(*ptr != '\0' && (buffer - bstart) < bufsize) >> + while(*ptr != '\0' && (buffer - bstart) < (int)bufsize) >> { >> if(*ptr == '%') { >> char formstr[20]; >> diff --git a/lib/libc/string/memmove.c b/lib/libc/string/memmove.c >> index 3acf1a973bbe..9d0962847296 100644 >> --- a/lib/libc/string/memmove.c >> +++ b/lib/libc/string/memmove.c >> @@ -18,7 +18,7 @@ memmove(void *dest, const void *src, size_t n) >> { >> char *cdest; >> const char *csrc; >> - int i; >> + size_t i; >> /* Do the buffers overlap in a bad way? */ >> if (src < dest && src + n >= dest) { >> >
On 28/01/2021 04.29, Alexey Kardashevskiy wrote: > > > On 27/01/2021 23:58, Thomas Huth wrote: >> On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >>> -Wextra enables a bunch of rather useful checks which this fixes. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> lib/libc/stdio/vsnprintf.c | 15 ++++++++------- >>> lib/libc/string/memmove.c | 2 +- >>> 2 files changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c >>> index 21dd04dfe46f..1a44301f74da 100644 >>> --- a/lib/libc/stdio/vsnprintf.c >>> +++ b/lib/libc/stdio/vsnprintf.c >>> @@ -25,15 +25,15 @@ static int >>> print_str_fill(char **buffer, size_t bufsize, char *sizec, >>> const char *str, char c) >>> { >>> - int i, sizei, len; >>> + unsigned i, sizei, len; >>> char *bstart = *buffer; >>> sizei = strtoul(sizec, NULL, 10); >>> len = strlen(str); >>> if (sizei > len) { >>> for (i = 0; >>> - (i < (sizei - len)) && ((*buffer - bstart) < bufsize); >>> - i++) { >>> + (i < (sizei - len)) && ((*buffer - bstart) < (int)bufsize); >> >> bufsize is of type size_t, so the correct way to cast would be "(ssize_t)" >> instead of "(int)", I think. >> Maybe it would also be worth the effort to introduce a new local variable >> a la: >> >> ssize_t sbufsize = (ssize_t)bufsize; >> >> so that you don't have to do the cast again and again all over the place? > > > This is done once per a function, does not seem worth the variable, unless I > missed something? Thanks, Oh, well, sorry, I misread the patch, thought that the hunks related to the same functions. So never mind! Thomas
diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c index 21dd04dfe46f..1a44301f74da 100644 --- a/lib/libc/stdio/vsnprintf.c +++ b/lib/libc/stdio/vsnprintf.c @@ -25,15 +25,15 @@ static int print_str_fill(char **buffer, size_t bufsize, char *sizec, const char *str, char c) { - int i, sizei, len; + unsigned i, sizei, len; char *bstart = *buffer; sizei = strtoul(sizec, NULL, 10); len = strlen(str); if (sizei > len) { for (i = 0; - (i < (sizei - len)) && ((*buffer - bstart) < bufsize); - i++) { + (i < (sizei - len)) && ((*buffer - bstart) < (int)bufsize); + i++) { **buffer = c; *buffer += 1; } @@ -47,7 +47,7 @@ print_str(char **buffer, size_t bufsize, const char *str) char *bstart = *buffer; size_t i; - for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < bufsize); i++) { + for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < (int)bufsize); i++) { **buffer = str[i]; *buffer += 1; } @@ -112,7 +112,7 @@ print_fill(char **buffer, size_t bufsize, char *sizec, unsigned long size, len = print_intlen(size, base) + optlen; if (sizei > len) { for (i = 0; - (i < (sizei - len)) && ((*buffer - bstart) < bufsize); + (i < (sizei - len)) && ((*buffer - bstart) < (int)bufsize); i++) { **buffer = c; *buffer += 1; @@ -143,7 +143,7 @@ print_format(char **buffer, size_t bufsize, const char *format, void *var) form++; } - while ((*form != '\0') && ((*buffer - start) < bufsize)) { + while ((*form != '\0') && ((*buffer - start) < (int)bufsize)) { switch(*form) { case 'u': case 'd': @@ -163,6 +163,7 @@ print_format(char **buffer, size_t bufsize, const char *format, void *var) break; case 'X': upper = true; + /* fallthrough */ case 'x': sizec[i] = '\0'; value = (unsigned long) var & convert[length_mod]; @@ -260,7 +261,7 @@ vsnprintf(char *buffer, size_t bufsize, const char *format, va_list arg) /* Leave one space for NULL character */ bufsize--; - while(*ptr != '\0' && (buffer - bstart) < bufsize) + while(*ptr != '\0' && (buffer - bstart) < (int)bufsize) { if(*ptr == '%') { char formstr[20]; diff --git a/lib/libc/string/memmove.c b/lib/libc/string/memmove.c index 3acf1a973bbe..9d0962847296 100644 --- a/lib/libc/string/memmove.c +++ b/lib/libc/string/memmove.c @@ -18,7 +18,7 @@ memmove(void *dest, const void *src, size_t n) { char *cdest; const char *csrc; - int i; + size_t i; /* Do the buffers overlap in a bad way? */ if (src < dest && src + n >= dest) {
-Wextra enables a bunch of rather useful checks which this fixes. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- lib/libc/stdio/vsnprintf.c | 15 ++++++++------- lib/libc/string/memmove.c | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-)