Date: Sun, 14 Mar 2010 22:26:11 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <yanefbsd@gmail.com> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Cran <brucec@FreeBSD.org> Subject: Re: svn commit: r205118 - head/sbin/sysctl Message-ID: <20100314212000.N23495@delplex.bde.org> In-Reply-To: <7d6fde3d1003131313l3559b6dbt1566fb23be150408@mail.gmail.com> References: <201003131108.o2DB8vmu001454@svn.freebsd.org> <7d6fde3d1003131313l3559b6dbt1566fb23be150408@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--0-2088975880-1268565971=:23495
Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
On Sat, 13 Mar 2010, Garrett Cooper wrote:
>> Log:
>> =A0Free the memory allocated via strdup.
Why bother?
1. Memory is infinite :-).
2. Memory is infinite relative to sysctl's requirements, especially for
this strdup. Normally this strdup is never reached. It is normally
reached a whole 1 time for sysctl -a (e.g., on ref9-amd64 now),
since there is a whole 1 sysctl with a timeval format in the configured
kernel, and not many more (if any more) than 1 in /sys.
>> Modified: head/sbin/sysctl/sysctl.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
>> --- head/sbin/sysctl/sysctl.c =A0 Sat Mar 13 11:06:47 2010 =A0 =A0 =A0 =
=A0(r205117)
>> +++ head/sbin/sysctl/sysctl.c =A0 Sat Mar 13 11:08:57 2010 =A0 =A0 =A0 =
=A0(r205118)
>> @@ -382,6 +382,7 @@ S_timeval(int l2, void *p)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (*p2 =3D=3D '\n')
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*p2 =3D '\0';
>> =A0 =A0 =A0 =A0fputs(p1, stdout);
>> + =A0 =A0 =A0 free(p1);
>> =A0 =A0 =A0 =A0return (0);
>> =A0}
The \xa0's are ugly.
> strdup(3) can fail in that section of code, thus the fputs(3)
> could segfault:
Who cares?
1. strdup() can't fail, since memory is infinite :-)
2. ... since memory is infinite relative to sysctl's requirements (see abov=
e).
3. Most systems are (mis)configured with MALLOC_OPTIONS=3DA..., so if strdu=
p()
fails it doesn't return, and all the careful code that checks
malloc() and strdup() for failing becomes just an obfuscation.
MALLOC_OPTIONS=3DA... seriously breaks programs that are likely to
run out of memory and handle this, but such programs became rare when
address spaces exceeded 64K and rarer when they exceeded 640K...; they
are now infinite :-), so problems are rarely seen.
The strdup() is bogus anyway. ctime() returns a non-const string so the
the string should be modified directly. It is just as easy to avoid the
modification. The function with this has lots more style bugs:
% static int
% S_timeval(int l2, void *p)
Style bug: *p should be const.
% {
% =09struct timeval *tv =3D (struct timeval*)p;
Style bug: initialization in declaration.
% =09time_t tv_sec;
% =09char *p1, *p2;
%=20
% =09if (l2 !=3D sizeof(*tv)) {
% =09=09warnx("S_timeval %d !=3D %zu", l2, sizeof(*tv));
% =09=09return (1);
% =09}
% =09printf(hflag ? "{ sec =3D %'jd, usec =3D %'ld } " :
% =09=09"{ sec =3D %jd, usec =3D %ld } ",
% =09=09(intmax_t)tv->tv_sec, tv->tv_usec);
Style bugs: non-KNF indentation of continued lines.
% =09tv_sec =3D tv->tv_sec;
% =09p1 =3D strdup(ctime(&tv_sec));
Style bug: strdup() just adds bloat (especially when you add memory
management and error handling for it).
% =09for (p2=3Dp1; *p2 ; p2++)
Style bug: missing spaces around binary operator.
% =09=09if (*p2 =3D=3D '\n')
% =09=09=09*p2 =3D '\0';
To avoid the modification, break here and print the (p2 - p1) characters
starting at p1 using "%.*s" format. This leads to complete de-bloatation:
1: the newline is always at the end, so it can be located using strlen()
2: strlen() is unnecessary to, since the string always has length 25.
Thus "%.*s", ..., [p2 - p1 OR strlen(p1) -1] reduces to "%.24s".
% =09fputs(p1, stdout);
There is also a style rule against using fputs()...
% =09free(p1);
So the above lines (here they are again, indented a bit, with some error
handling added for not-quite maximal verboseness):
=09% =09p1 =3D strdup(ctime(&tv_sec));
=09%=09if (p1 =3D=3D NULL)
=09%=09=09err(1, "strdup");
=09% =09for (p2=3Dp1; *p2 ; p2++)
=09% =09=09if (*p2 =3D=3D '\n')
=09% =09=09=09*p2 =3D '\0';
=09% =09fputs(p1, stdout);
=09% =09free(p1);
are a verbose way of writing:
=09printf("%.24s", ctime(&tv_sec));
To reduce verboseness a little more, merge this printf() with the preceding
one.
To increase verboseness for both, add error handling for fputs() and printf=
().
% =09return (0);
% }
--0-2088975880-1268565971=:23495--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100314212000.N23495>