netdev - Re: [PATCH iproute2] json_print: fix print

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <145C0560-F26C-4A45-9E2A-009C822E9123@darbyshire-bryant.me.uk>
Date:   Thu, 29 Mar 2018 21:29:30 +0000
From:   Kevin Darbyshire-Bryant <kevin@...byshire-bryant.me.uk>
To:     Stephen Hemminger <stephen@...workplumber.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH iproute2] json_print: fix print_uint hidden type promotion



> On 29 Mar 2018, at 22:02, Stephen Hemminger <stephen@...workplumber.org> wrote:
> 
> On Thu, 29 Mar 2018 20:22:20 +0100
> Kevin Darbyshire-Bryant <ldir@...byshire-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@...byshire-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 :-)