resolve format string warnings

PATCH In review Ilya Shipitsin 4 participants 10 messages Mar 25, 2017 – Mar 28, 2017

Share & tools

Ilya Shipitsin

author

2017-03-25 12:21 UTC #0 lore raw

PATCH 1/1 resolve format string warnings

[src/openvpn/options.c:1163]: (warning) %x in format string (no. 1) requires 'unsigned int *' but the argument type is 'signed int *'. [src/openvpn/route.c:3187]: (warning) %d in format string (no. 5) requires 'int *' but the argument type is 'unsigned int *'.

Signed-off-by Ilya Shipitsin <[email protected]...>

---
 src/openvpn/options.c | 2 +-
 src/openvpn/route.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

src/openvpn/options.c +1 -1

index dcb6ecf..2bc7e62 100644
@@ -1147,7 +1147,7 @@ parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_aren
     const char *cp = str;
     uint8_t *ret = (uint8_t *) gc_malloc(nbytes, true, gc);
     char term = 1;
-    int byte;
+    unsigned int byte;
     char bs[3];
 
     for (i = 0; i < nbytes; ++i)

src/openvpn/route.c +1 -1

index 08998d5..ecfa60f 100644
@@ -3184,7 +3184,7 @@ get_default_gateway(struct route_gateway_info *rgi)
                     unsigned int flags = 0;
                     char name[16];
                     name[0] = 0;
-                    const int np = sscanf(line, "%15s\t%x\t%x\t%x\t%*s\t%*s\t%d\t%x",
+                    const int np = sscanf(line, "%15s\t%x\t%x\t%x\t%*s\t%*s\t%u\t%x",
                                           name,
                                           &net_x,
                                           &gw_x,

Signature · 1 line

2.5.5

Gert Doering

2017-03-25 13:17 UTC #1 lore raw

Hi,

Quoted 1 line · Ilya Shipitsin

[src/openvpn/options.c:1163]: (warning) %x in format string (no. 1) requires 'unsigned int *' but the argument type is 'signed int *'.

Please use "uint8_t" then, and also adapt the "ret[i] = (uint8_t)byte" line - the cast would then be obsolete.

Fixing compiler warnings needs to be done with a view of the surrounding context, otherwise it just makes complicated and inconsistant code.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

David Sommerseth

2017-03-27 21:40 UTC #2 lore raw

Quoted 7 lines · Gert Doering

Hi,
Quoted 1 line · Ilya Shipitsin
[src/openvpn/options.c:1163]: (warning) %x in format string (no. 1) requires 'unsigned int *' but the argument type is 'signed int *'.
Please use "uint8_t" then, and also adapt the "ret[i] = (uint8_t)byte" line - the cast would then be obsolete.

I would normally agree to use uint8_t ... but the man page for sscanf() on Linux says:

"Matches an unsigned hexadecimal integer; the next pointer must be a
pointer to unsigned int."

And stdint.h declares: typedef unsigned char uint8_t;

AFAIR, char is defined as 8 bits (hence uint*8*), while unsigned int are 32 bits on my system.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Technologies, Inc

Simon Ruderich

2017-03-27 22:04 UTC #3 lore raw

Quoted 12 lines · David Sommerseth

Quoted 7 lines · Gert Doering
Hi,
Quoted 1 line · Ilya Shipitsin
[src/openvpn/options.c:1163]: (warning) %x in format string (no. 1) requires 'unsigned int *' but the argument type is 'signed int *'.
Please use "uint8_t" then, and also adapt the "ret[i] = (uint8_t)byte" line - the cast would then be obsolete.
I would normally agree to use uint8_t ... but the man page for sscanf() on Linux says:

Quoted 7 lines

"Matches an unsigned hexadecimal integer; the next pointer must be a
pointer to unsigned int."
And stdint.h declares: typedef unsigned char uint8_t;
AFAIR, char is defined as 8 bits (hence uint*8*), while unsigned int are 32 bits on my system.

man inttypes.h yields the necessary constant for uint8_t : SCNu8

Regards
Simon

Signature · 3 lines

+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Gert Doering

2017-03-28 06:04 UTC #4 lore raw

In reply to David Sommerseth

Hi,

Quoted 9 lines · David Sommerseth

Quoted 5 lines · Ilya Shipitsin
Quoted 1 line
[src/openvpn/options.c:1163]: (warning) %x in format string (no. 1) requires 'unsigned int *' but the argument type is 'signed int *'.
Please use "uint8_t" then, and also adapt the "ret[i] = (uint8_t)byte" line - the cast would then be obsolete.
I would normally agree to use uint8_t ... but the man page for sscanf() on Linux says:

Quoted 7 lines

"Matches an unsigned hexadecimal integer; the next pointer must be a
pointer to unsigned int."
And stdint.h declares: typedef unsigned char uint8_t;
AFAIR, char is defined as 8 bits (hence uint*8*), while unsigned int are 32 bits on my system.

Yeah, you're right. I confused this discussion with our standard "char vs. signed char vs. unsigned char" warning.

(See how dangerous "fixing compiler warnings" is? :-) )

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Илья Шипицин

author

2017-03-28 06:43 UTC #5 lore raw

2017-03-28 11:04 GMT+05:00 Gert Doering <[email protected]...>:

Quoted 18 lines

Hi,
Quoted 2 lines · David Sommerseth
Quoted 2 lines · Ilya Shipitsin
Quoted 1 line
[src/openvpn/options.c:1163]: (warning) %x in format string (no. 1)
requires 'unsigned int *' but the argument type is 'signed int *'.
Quoted 12 lines
Quoted 3 lines
Please use "uint8_t" then, and also adapt the "ret[i] = (uint8_t)byte" line - the cast would then be obsolete.
I would normally agree to use uint8_t ... but the man page for sscanf() on Linux says:
"Matches an unsigned hexadecimal integer; the next pointer must be a
pointer to unsigned int."
And stdint.h declares: typedef unsigned char uint8_t;

Quoted 8 lines

Quoted 2 lines
AFAIR, char is defined as 8 bits (hence uint*8*), while unsigned int are 32 bits on my system.
Yeah, you're right. I confused this discussion with our standard "char vs. signed char vs. unsigned char" warning.
(See how dangerous "fixing compiler warnings" is? :-) )

this particular warning came from cppcheck, it's not a compiler warning

I hope to address compiler warnings after "travis-ci + mingw" patch applied. there are a lot of warnings in mingw builds:

Quoted 11 lines

gert
--
USENET is *not* the non-clickable part of WWW!
                                                           //
www.muc.de/~gert/ <http://www.muc.de/%7Egert/>
Gert Doering - Munich, Germany
[email protected]...
fax: +49-89-35655025 [email protected]...
muenchen.de

Gert Doering

2017-03-28 06:56 UTC #6 lore raw

Hi,

Quoted 3 lines · ???????? ??????????????

Quoted 1 line
(See how dangerous "fixing compiler warnings" is? :-) )
this particular warning came from cppcheck, it's not a compiler warning

Same thing. A tool that issues a warning which is not relevant the way the variables in question are used (because the values stored are always way below where signed/unsiged gets relevant) - but trying to fix it, I've misread the surrounding code and the "fixed" code would quite likely have crashed, or silently corrupted nearby variables.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Илья Шипицин

author

2017-03-28 07:04 UTC #7 lore raw

2017-03-28 11:56 GMT+05:00 Gert Doering <[email protected]...>:

Quoted 5 lines

Hi,
Quoted 2 lines · ???????? ??????????????
Quoted 1 line
(See how dangerous "fixing compiler warnings" is? :-) )

Quoted 9 lines

Quoted 1 line
this particular warning came from cppcheck, it's not a compiler warning
Same thing. A tool that issues a warning which is not relevant the way the variables in question are used (because the values stored are always way below where signed/unsiged gets relevant) - but trying to fix it, I've misread the surrounding code and the "fixed" code would quite likely have crashed, or silently corrupted nearby variables.

cppcheck says "variable is signed, but format specification is unsigned" so, I had a look around, it is "byte" variable, which cannot be signed, so I decided to change it "int" --> "unsigned int"

later, Gert Doering suggested to switch to uit8_t

probably, we need to leave variable as is, just change format specification "%x" --> "%d" ?

Quoted 10 lines

gert
--
USENET is *not* the non-clickable part of WWW!
                                                           //
www.muc.de/~gert/
Gert Doering - Munich, Germany
[email protected]...
fax: +49-89-35655025 [email protected]...
muenchen.de

David Sommerseth

2017-03-28 12:54 UTC #8 lore raw

Quoted 19 lines · Илья Шипицин

2017-03-28 11:56 GMT+05:00 Gert Doering <[email protected]... <mailto:[email protected]...>>:
Hi,
On Tue, Mar 28, 2017 at 11:43:11AM +0500, ???????? ?????????????? wrote:
> > (See how dangerous "fixing compiler warnings" is? :-) ) >
> this particular warning came from cppcheck, it's not a compiler warning
Same thing. A tool that issues a warning which is not relevant the way the variables in question are used (because the values stored are always way below where signed/unsiged gets relevant) - but trying to fix it, I've misread the surrounding code and the "fixed" code would quite likely have crashed, or silently corrupted nearby variables.

Quoted 8 lines

cppcheck says "variable is signed, but format specification is unsigned" so, I had a look around, it is "byte" variable, which cannot be signed, so I decided to change it "int" --> "unsigned int"
later, Gert Doering suggested to switch to uit8_t
probably, we need to leave variable as is, just change format specification "%x" --> "%d" ?

Take a moment to re-read what I already quoted from the man page for sscanf() related to %x:

"Matches an unsigned hexadecimal integer; the next pointer must be a
pointer to unsigned int."

Gert proposed by a mistake to use uint8_t. As integers are most commonly 32 bits, so uint32_t. But that may be of a different length on some other platforms. So using 'unsigned int' is most likely the safest type in this round. Or if wanting to be really portability safe, have a look at what Simon pointed out, inttypes.h.

Changing from %x to %d will definitely cause havoc. This function [parse_hash_fingerprint()] is related to parsing fingerprint hashes into an integer value. Certificate fingerprint hashes are most commonly longer strings of hexadecimals. So such a change will actually make sscanf() fail horrendously when the characters a-f appears. %d does not handle hexadecimal numbers.

Again, as Gert have already said twice ... it is important to get an understanding of what the code does before trying to change it. Code analysers doesn't necessarily provide the only sane truth, as most of them is incapable of understanding the broader context. I do not say such analysers are useless or pointless, merely that each warning must first be thoroughly reviewed and considered before just jumping into changing things.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Technologies, Inc

Gert Doering

2017-03-28 12:58 UTC #9 lore raw

Hi,

Quoted 5 lines · David Sommerseth

Gert proposed by a mistake to use uint8_t. As integers are most commonly 32 bits, so uint32_t. But that may be of a different length on some other platforms. So using 'unsigned int' is most likely the safest type in this round. Or if wanting to be really portability safe, have a look at what Simon pointed out, inttypes.h.

"unsigned int" is indeed perfectly correct for "%x"

I was not paying enough attention (and this is why I call for extra care when doing "just a simple patch to fix a compiler warning", because more complicated patches usually get more attention...)

Apologies for the noise I caused. Actually the *first* patch in the series might have been perfectly fine - need to check ;-)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...