NetBSD Problem Report #54053

From msaitoh@five.execsw.org  Mon Mar 11 06:20:07 2019
Return-Path: <msaitoh@five.execsw.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 27C137A1A2
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 11 Mar 2019 06:20:07 +0000 (UTC)
Message-Id: <20190311062004.8358788612A@five.execsw.org>
Date: Mon, 11 Mar 2019 15:20:04 +0900 (JST)
From: msaitoh@execsw.org
Reply-To: msaitoh@execsw.org
To: gnats-bugs@NetBSD.org
Subject: humanize_number(HN_AUTOSCALE) with big buffer doesn't work.
X-Send-Pr-Version: 3.95

>Number:         54053
>Category:       lib
>Synopsis:       humanize_number(HN_AUTOSCALE) with big buffer doesn't work.
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kre
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Mar 11 06:25:00 +0000 2019
>Closed-Date:    Wed Mar 13 05:46:29 +0000 2019
>Last-Modified:  Wed Mar 13 05:46:29 +0000 2019
>Originator:     msaitoh@execsw.org
>Release:        Any?
>Organization:
>Environment:
>Description:
	It seems using humanize_number(HN_AUTOSCALE) with big buffer doesn't work as expected.

------------------ test program ---------------
#include <assert.h>
#include <inttypes.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#ifdef __FreeBSD__
#include <libutil.h>
#endif

/*
 * bufsize	bps		result
 *
 *	40	1000000000	0 Ebps
 *	39	1000000000	1000000000 bps
 */
int
main(int argc, char *argv[])
{

	char buf[40];
	int64_t bps = 1000000000;
	int rv;

	rv = humanize_number(buf, sizeof(buf), bps, "bps",
	     HN_AUTOSCALE, HN_DIVISOR_1000);
	printf("\tbaudrate %s\n", buf);

	return rv;
}
-----------------------------------------

	1000000000 with buffer size 39 works:

		1000000000 bps

	1000000000 with buffer size 40 doesn't work:

		0 Ebps

	I added debug printf into humanize_number.c like below:

-------------------------------------
	/* Check if enough room for `x y' + suffix + `\0' */
	if (len < baselen + 1)
		return (-1);
#if 1
	printf("divisor = %ld\n", divisor);
	printf("    len = %ld\n", len);
	printf("baselen = %ld\n", baselen);
	printf("bytes = %ld\n", bytes);
#endif
	if (scale & (HN_AUTOSCALE | HN_GETSCALE)) {
		/* See if there is additional columns can be used. */
		for (max = 100, i = len - baselen; i-- > 0;) {
			printf("i = %zu, max   = %ld\n", i, max);
			max *= 10;
		}
-------------------------------------

	The output was:

./humanize_number
divisor = 1000
    len = 40
baselen = 6
bytes = 100000000000
i = 33, max   = 100
i = 32, max   = 1000
i = 31, max   = 10000
i = 30, max   = 100000
i = 29, max   = 1000000
i = 28, max   = 10000000
i = 27, max   = 100000000
i = 26, max   = 1000000000
i = 25, max   = 10000000000
i = 24, max   = 100000000000
i = 23, max   = 1000000000000
i = 22, max   = 10000000000000
i = 21, max   = 100000000000000
i = 20, max   = 1000000000000000
i = 19, max   = 10000000000000000
i = 18, max   = 100000000000000000
i = 17, max   = 1000000000000000000
i = 16, max   = -8446744073709551616
i = 15, max   = 7766279631452241920
i = 14, max   = 3875820019684212736
i = 13, max   = 1864712049423024128
i = 12, max   = 200376420520689664
i = 11, max   = 2003764205206896640
i = 10, max   = 1590897978359414784
i = 9, max   = -2537764290115403776
i = 8, max   = -6930898827444486144
i = 7, max   = 4477988020393345024
i = 6, max   = 7886392056514347008
i = 5, max   = 5076944270305263616
i = 4, max   = -4570789518076018688
i = 3, max   = -8814407033341083648
i = 2, max   = 4089650035136921600
i = 1, max   = 4003012203950112768
i = 0, max   = 3136633892082024448
        baudrate 0 Ebps

	Overflow?

>How-To-Repeat:
	See above.
>Fix:
	I don't know.

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: lib-bug-people->kre
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Mon, 11 Mar 2019 08:52:05 +0000
Responsible-Changed-Why:
I am looking into this PR


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54053 CVS commit: src/lib/libc/gen
Date: Mon, 11 Mar 2019 15:10:52 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Mon Mar 11 15:10:51 UTC 2019

 Modified Files:
 	src/lib/libc/gen: humanize_number.3 humanize_number.c

 Log Message:
 PR lib/54053

 When auto scaling, and the buffer is bigger than big enough
 for the biggest possible number, don't try and calculate
 the max value that will fit in the buffer - that calc
 will overflow (guaranteed) and is useless, the value
 we're formatting cannot possibly be bigger.  So simply
 use the unscaled value (the raw number).

 While here, also avoid returning values that are larger
 than the buffer len ... while it would be nice to be able
 to find out how big the buffer should be so the data will
 fit, the interface doesn't really allow that (the buffer
 length passed in controls the scaling - at least when
 auto scaling) and the code already does "return -1" when
 it detects the buffer length is too small, even before
 it works out how much would have been needed.  So, rather
 than returning a value > len (while truncating the result
 to fit in len ... all courtesy of snprintf()) return -1
 in this case as well.

 Also, allow suffix==NULL (meaning "") - there's no reason
 not to, and requiring users to pass in an explicit "" is
 not useful.


 To generate a diff of this commit:
 cvs rdiff -u -r1.11 -r1.12 src/lib/libc/gen/humanize_number.3
 cvs rdiff -u -r1.17 -r1.18 src/lib/libc/gen/humanize_number.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: kre@NetBSD.org
State-Changed-When: Mon, 11 Mar 2019 15:15:52 +0000
State-Changed-Why:
Can you verify if this is fixed OK now (in HEAD, as of a minute ago...)

Also, do you need this pulled up to -8 ?   humanize_number() seems to
have been untouched in any signifigant way in a very long time, and
while this was a fairly blatant bug, it had not previously been noticed.


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54053 CVS commit: src/tests/lib/libc/gen
Date: Mon, 11 Mar 2019 17:45:12 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Mon Mar 11 17:45:12 UTC 2019

 Modified Files:
 	src/tests/lib/libc/gen: t_humanize_number.c

 Log Message:
 Explicitly test for PR lib/54053

 A suitable test was actually there already - but the results
 were not verified.   So just add a test that the result string
 is what is expected.  (Previously for len==128 and bytes==10000
 it would  have returned "0E" now it returns 10000 as it should.)


 To generate a diff of this commit:
 cvs rdiff -u -r1.9 -r1.10 src/tests/lib/libc/gen/t_humanize_number.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54053 CVS commit: src/sys/kern
Date: Tue, 12 Mar 2019 00:25:44 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Tue Mar 12 00:25:44 UTC 2019

 Modified Files:
 	src/sys/kern: subr_humanize.c

 Log Message:
 This had a similar problem to that reported in PR lib/54053
 for the userland (libc) version of humanize_number(),
 except in a much more limited, and less harmful, way ...

 If the value to be printed was > ~0/10 (eg ~0 itself) and
 the buffer given was big enough to hold it, "umax" would overflow,
 and the expected result was not produced (here at least the effect was
 simply to scale the result for huge values by one unnecessary step,
 rather than maximally scale all values usually generating 0E, but
 incorrect nonetheless.)


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/sys/kern/subr_humanize.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Masanobu SAITOH <msaitoh@execsw.org>
To: gnats-bugs@NetBSD.org, kre@NetBSD.org, netbsd-bugs@netbsd.org,
 gnats-admin@netbsd.org
Cc: msaitoh@execsw.org
Subject: Re: lib/54053 (humanize_number(HN_AUTOSCALE) with big buffer doesn't
 work.)
Date: Wed, 13 Mar 2019 14:10:15 +0900

 On 2019/03/12 0:15, kre@NetBSD.org wrote:
 > Synopsis: humanize_number(HN_AUTOSCALE) with big buffer doesn't work.
 > 
 > State-Changed-From-To: open->feedback
 > State-Changed-By: kre@NetBSD.org
 > State-Changed-When: Mon, 11 Mar 2019 15:15:52 +0000
 > State-Changed-Why:
 > Can you verify if this is fixed OK now (in HEAD, as of a minute ago...)

 Yes. It's fixed!

 > Also, do you need this pulled up to -8 ?   humanize_number() seems to
 > have been untouched in any signifigant way in a very long time, and
 > while this was a fairly blatant bug, it had not previously been noticed.

   This bug is not serious and it's not required for me to pullup to -8.
 But, another person might get the same problem and might waste the time.
 It would be good to pullup to avoid it and reduce diff between
 -current and netbsd-[78].

 -- 
 -----------------------------------------------
                  SAITOH Masanobu (msaitoh@execsw.org
                                   msaitoh@netbsd.org)

From: Robert Elz <kre@munnari.OZ.AU>
To: Masanobu SAITOH <msaitoh@execsw.org>
Cc: gnats-bugs@NetBSD.org, netbsd-bugs@netbsd.org
Subject: Re: lib/54053 (humanize_number(HN_AUTOSCALE) with big buffer doesn't work.)
Date: Wed, 13 Mar 2019 12:34:50 +0700

     Date:        Wed, 13 Mar 2019 14:10:15 +0900
     From:        Masanobu SAITOH <msaitoh@execsw.org>
     Message-ID:  <11147520-0be5-fb8a-aa5f-aa4aff29dca9@execsw.org>

   | Yes. It's fixed!

 Thanks for confirming.

   |   This bug is not serious and it's not required for me to pullup to -8.

 OK, in that case I think I won't bother as ...

   | But, another person might get the same problem and might waste the time.
   | It would be good to pullup to avoid it and reduce diff between
   | -current and netbsd-[78].

 First I think it better to reduce the diff between 8.0 and 8_STABLE
 (and 8.1 when it happens) than between 8 and CURRENT (or 9).  The latter
 is pretty much a waste of time (there's LOTS of diff).

 Second, I think it's unlikely that anyone else will ever see this
 problem (and I wonder at just what you were really doing - other than
 the illustration demo included in the PR - when you experienced this
 problem.)   HN_AUTOSCALE and a big buffer together do not really make
 sense - HN_AUTOSCALE is intended to make the number fit in a limited
 field width, if all possible - if the buffer is any larger than (about)
 20 + the length of the suffix to be added, you might just as well simply
 do snprintf() and be done with it, as that is all that humanize_number()
 does in that case.

 That will be why, that even though this bug had been there essentially
 forever (decades...) no-one had previously noticed.

 I wouldn't bother with a pullup to -7 in any case.  I doubt at this stage
 that there will ever be a new point release for that, and this issue just
 isn't important enough.   It would only get to those running 7_STABLE
 (and upgrading that from time to time) which are a set of people who at
 this stage are probably much better off running 8_STABLE instead.

 But if you really want to request a pullup to -8 by all means go ahead.
 (I can supply the needed source-changes message if you don't still have it).
 The patch should apply easily enogh I'd think (and if there were any other
 changes, simply including all of them - pulling -8 up to the same as HEAD
 would not be unreasonable.)

 For now, I will just close this though.

 kre


State-Changed-From-To: feedback->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Wed, 13 Mar 2019 05:46:29 +0000
State-Changed-Why:
Submitter confirms that problem is fixed


>Unformatted:

Home	PR Database Search