| Message ID | 20180329192220.30101-1-ldir@darbyshire-bryant.me.uk |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] json_print: fix print_uint hidden type promotion | expand |
On Thu, 29 Mar 2018 20:22:20 +0100 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> wrote: > print_int used 'int' type internally, whereas print_uint used 'uint64_t' > > These helper functions eventually call vfprintf(fp, fmt, args) which is > a variable argument list function and is dependent upon 'fmt' containing > correct information about the length of the passed arguments. > > Unfortunately print_int v print_uint offered no clue to the programmer > that internally passed ints to print_uint were being promoted to 64bits, > thus the format passed in 'fmt' string vs the actual passed integer > could be different lengths. This is even more interesting on big endian > architectures where 'vfprintf' would be looking in the middle of an > int64 type. > > print_u/int now stick with native int size. > > Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> > --- > include/json_print.h | 2 +- > lib/json_print.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/json_print.h b/include/json_print.h > index 2ca7830a..45bc653d 100644 > --- a/include/json_print.h > +++ b/include/json_print.h > @@ -56,10 +56,10 @@ void close_json_array(enum output_type type, const char *delim); > print_color_##type_name(t, COLOR_NONE, key, fmt, value); \ > } > _PRINT_FUNC(int, int); > +_PRINT_FUNC(uint, unsigned int); > _PRINT_FUNC(bool, bool); > _PRINT_FUNC(null, const char*); > _PRINT_FUNC(string, const char*); > -_PRINT_FUNC(uint, uint64_t); > _PRINT_FUNC(hu, unsigned short); > _PRINT_FUNC(hex, unsigned int); > _PRINT_FUNC(0xhex, unsigned int); > diff --git a/lib/json_print.c b/lib/json_print.c > index 6518ba98..8d54d1d4 100644 > --- a/lib/json_print.c > +++ b/lib/json_print.c > @@ -117,8 +117,8 @@ void close_json_array(enum output_type type, const char *str) > } \ > } > _PRINT_FUNC(int, int); > +_PRINT_FUNC(uint, unsigned int); > _PRINT_FUNC(hu, unsigned short); > -_PRINT_FUNC(uint, uint64_t); > _PRINT_FUNC(lluint, unsigned long long int); > _PRINT_FUNC(float, double); > #undef _PRINT_FUNC I am concerned that this will break output of 64 bit statistics on 32 bit hosts.
> On 29 Mar 2018, at 22:02, Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Thu, 29 Mar 2018 20:22:20 +0100 > Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> wrote: > >> print_int used 'int' type internally, whereas print_uint used 'uint64_t' >> >> These helper functions eventually call vfprintf(fp, fmt, args) which is >> a variable argument list function and is dependent upon 'fmt' containing >> correct information about the length of the passed arguments. >> >> Unfortunately print_int v print_uint offered no clue to the programmer >> that internally passed ints to print_uint were being promoted to 64bits, >> thus the format passed in 'fmt' string vs the actual passed integer >> could be different lengths. This is even more interesting on big endian >> architectures where 'vfprintf' would be looking in the middle of an >> int64 type. >> >> print_u/int now stick with native int size. >> >> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> >> --- >> include/json_print.h | 2 +- >> lib/json_print.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/json_print.h b/include/json_print.h >> index 2ca7830a..45bc653d 100644 >> --- a/include/json_print.h >> +++ b/include/json_print.h >> @@ -56,10 +56,10 @@ void close_json_array(enum output_type type, const char *delim); >> print_color_##type_name(t, COLOR_NONE, key, fmt, value); \ >> } >> _PRINT_FUNC(int, int); >> +_PRINT_FUNC(uint, unsigned int); >> _PRINT_FUNC(bool, bool); >> _PRINT_FUNC(null, const char*); >> _PRINT_FUNC(string, const char*); >> -_PRINT_FUNC(uint, uint64_t); >> _PRINT_FUNC(hu, unsigned short); >> _PRINT_FUNC(hex, unsigned int); >> _PRINT_FUNC(0xhex, unsigned int); >> diff --git a/lib/json_print.c b/lib/json_print.c >> index 6518ba98..8d54d1d4 100644 >> --- a/lib/json_print.c >> +++ b/lib/json_print.c >> @@ -117,8 +117,8 @@ void close_json_array(enum output_type type, const char *str) >> } \ >> } >> _PRINT_FUNC(int, int); >> +_PRINT_FUNC(uint, unsigned int); >> _PRINT_FUNC(hu, unsigned short); >> -_PRINT_FUNC(uint, uint64_t); >> _PRINT_FUNC(lluint, unsigned long long int); >> _PRINT_FUNC(float, double); >> #undef _PRINT_FUNC > > > I am concerned that this will break output of 64 bit statistics on 32 bit hosts. I honestly don’t know what to do. Without the patch I see breakage on <33 bit stats with 32 bit big endian hosts ‘cos the printf formatting doesn’t know the type passed internally by the function is 64bits long. e.g. tc qdisc qdisc noqueue 0: dev lo root refcnt 4486716 qdisc fq_codel 0: dev eth1 root refcnt 4486716 limit 4498840p flows 4536204 quantum 4539856 target 5.0ms interval 100.0ms memory_limit 4Mb ecn qdisc noqueue 0: dev br-lan root refcnt 4486716 qdisc noqueue 0: dev eth1.2 root refcnt 4486716 qdisc noqueue 0: dev br-wifi_guest root refcnt 4486716 qdisc noqueue 0: dev eth1.15 root refcnt 4486716 qdisc noqueue 0: dev wlan1 root refcnt 4486716 qdisc noqueue 0: dev wlan0 root refcnt 4486716 qdisc noqueue 0: dev wlan1-1 root refcnt 4486716 qdisc noqueue 0: dev wlan0-1 root refcnt 4486716 I guess _PRINT_FUNC(int, int) could be _PRINT_FUNC(int, int64_t) and then at least we’d be consistent in doing hidden promotions and see breakage for both signed & unsigned types on certain architectures. But I think I’ve hit my (lack of) skill limit and don’t really know how to take this further forward, or wish to break more protocols :-)
diff --git a/include/json_print.h b/include/json_print.h index 2ca7830a..45bc653d 100644 --- a/include/json_print.h +++ b/include/json_print.h @@ -56,10 +56,10 @@ void close_json_array(enum output_type type, const char *delim); print_color_##type_name(t, COLOR_NONE, key, fmt, value); \ } _PRINT_FUNC(int, int); +_PRINT_FUNC(uint, unsigned int); _PRINT_FUNC(bool, bool); _PRINT_FUNC(null, const char*); _PRINT_FUNC(string, const char*); -_PRINT_FUNC(uint, uint64_t); _PRINT_FUNC(hu, unsigned short); _PRINT_FUNC(hex, unsigned int); _PRINT_FUNC(0xhex, unsigned int); diff --git a/lib/json_print.c b/lib/json_print.c index 6518ba98..8d54d1d4 100644 --- a/lib/json_print.c +++ b/lib/json_print.c @@ -117,8 +117,8 @@ void close_json_array(enum output_type type, const char *str) } \ } _PRINT_FUNC(int, int); +_PRINT_FUNC(uint, unsigned int); _PRINT_FUNC(hu, unsigned short); -_PRINT_FUNC(uint, uint64_t); _PRINT_FUNC(lluint, unsigned long long int); _PRINT_FUNC(float, double); #undef _PRINT_FUNC
print_int used 'int' type internally, whereas print_uint used 'uint64_t' These helper functions eventually call vfprintf(fp, fmt, args) which is a variable argument list function and is dependent upon 'fmt' containing correct information about the length of the passed arguments. Unfortunately print_int v print_uint offered no clue to the programmer that internally passed ints to print_uint were being promoted to 64bits, thus the format passed in 'fmt' string vs the actual passed integer could be different lengths. This is even more interesting on big endian architectures where 'vfprintf' would be looking in the middle of an int64 type. print_u/int now stick with native int size. Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> --- include/json_print.h | 2 +- lib/json_print.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)