Bug 71628 - [patch] cleanup of the usr.sbin/rpcbind code
Summary: [patch] cleanup of the usr.sbin/rpcbind code
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 5.3-BETA3
Hardware: Any Any
: Normal Affects Some People
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-09-12 03:40 UTC by Dan Lukes
Modified: 2017-11-05 20:42 UTC (History)
1 user (show)

See Also:


Attachments
file.diff (838 bytes, patch)
2004-09-12 03:40 UTC, Dan Lukes
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Lukes 2004-09-12 03:40:28 UTC
	There are more than 5000 warnings issued during "make buildworld".
Some of them are false positives, but some of them are sign of true errors.

	Nobody is upset by warnings due it's amount, so some errors remain
uncorrected.

	I want to cleanup the code-base from warnings, so warnings will
become "attention mark" again.

usr.sbin/rpcbind/rpcbind.c:239: warning: 'fd' might be used uninitialized in this function
usr.sbin/rpcbind/rpcbind.c:243: warning: 'my_xprt' might be used uninitialized in this function
both variables are initialized before use with no exception

How-To-Repeat: 	N/A
Comment 1 Giorgos Keramidas freebsd_committer 2004-09-12 20:39:20 UTC
On 2004-09-12 04:38, Dan Lukes <dan@obluda.cz> wrote:
>   static int
>   init_transport(struct netconfig *nconf)
>   {
> - 	int fd;
> + 	int fd = fd;  /* init to suppres "may be used uninitialized" warning */

Err, no please.  GCC complains that fd "may be used uninitialized" because
of this construct near line 275 and the rest of the function body:

    275         if (nconf->nc_semantics != NC_TPI_CLTS) {
    276                 if ((fd = __rpc_nconf2fd(nconf)) < 0) {
    277                         int non_fatal = 0;
    ...
    283                         return (1);
    284                 }
    ...
    311         if (nconf->nc_semantics == NC_TPI_CLTS) {
    ...
    334                 while (nhostsbak > 0) {
    ...
    339                         if ((fd = __rpc_nconf2fd(nconf)) < 0) {
    ...
    452         } else {
    ...
    466                 if (bind(fd, sa, addrlen) < 0) {

Pay attention to this last line.  GCC believes that fd can be used
uninitialized because if NC_TPI_CLTS is not used the bind() call will
receive an uninitialized value of `fd'.  I'm not acquainted with the
internals of rpcbind() at all, but I know that Alfred Perlstein is.
He's probably the best person to suggest a fix for this warning, if one
is really needed.

>   	struct t_bind taddr;
>   	struct addrinfo hints, *res = NULL;
>   	struct __rpc_sockinfo si;
> - 	SVCXPRT	*my_xprt;
> ! 	SVCXPRT	*my_xprt = my_xprt; /* init to suppres "may be used uninitialized" warning */

No.  I don't know why you think that this is a good fix for all the
uninitialized pointer warnings.  It's not.  Never :-/

This warning is probably caused by GCC's inability to make sure that when
`bar' is a pointer this construct is perfectly fine:

	if (foo) {
		...
		if (bar = baz())
			...
	} else {
		...
		if (bar = baz2())
			...
	}

	if (blah(bar))
		...

After a quick glance at the source of rpcbind() I think that there's no way
that my_xrpt can be used uninitialized.  A proper fix for this warning
would be then to just set my_xrpt to NULL at first and let the rest of the
function unchanged regarding my_xrpt.
Comment 2 Alfred Perlstein freebsd_committer 2004-09-12 21:35:50 UTC
I'm having a hard time seeing these warnings, how are they
being triggered?

* Giorgos Keramidas <keramida@freebsd.org> [040912 13:07] wrote:
> On 2004-09-12 04:38, Dan Lukes <dan@obluda.cz> wrote:
> >   static int
> >   init_transport(struct netconfig *nconf)
> >   {
> > - 	int fd;
> > + 	int fd = fd;  /* init to suppres "may be used uninitialized" warning */
> 
> Err, no please.  GCC complains that fd "may be used uninitialized" because
> of this construct near line 275 and the rest of the function body:
> 
>     275         if (nconf->nc_semantics != NC_TPI_CLTS) {
>     276                 if ((fd = __rpc_nconf2fd(nconf)) < 0) {
>     277                         int non_fatal = 0;
>     ...
>     283                         return (1);
>     284                 }
>     ...
>     311         if (nconf->nc_semantics == NC_TPI_CLTS) {
>     ...
>     334                 while (nhostsbak > 0) {
>     ...
>     339                         if ((fd = __rpc_nconf2fd(nconf)) < 0) {
>     ...
>     452         } else {
>     ...
>     466                 if (bind(fd, sa, addrlen) < 0) {
> 
> Pay attention to this last line.  GCC believes that fd can be used
> uninitialized because if NC_TPI_CLTS is not used the bind() call will
> receive an uninitialized value of `fd'.  I'm not acquainted with the
> internals of rpcbind() at all, but I know that Alfred Perlstein is.
> He's probably the best person to suggest a fix for this warning, if one
> is really needed.
> 
> >   	struct t_bind taddr;
> >   	struct addrinfo hints, *res = NULL;
> >   	struct __rpc_sockinfo si;
> > - 	SVCXPRT	*my_xprt;
> > ! 	SVCXPRT	*my_xprt = my_xprt; /* init to suppres "may be used uninitialized" warning */
> 
> No.  I don't know why you think that this is a good fix for all the
> uninitialized pointer warnings.  It's not.  Never :-/
> 
> This warning is probably caused by GCC's inability to make sure that when
> `bar' is a pointer this construct is perfectly fine:
> 
> 	if (foo) {
> 		...
> 		if (bar = baz())
> 			...
> 	} else {
> 		...
> 		if (bar = baz2())
> 			...
> 	}
> 
> 	if (blah(bar))
> 		...
> 
> After a quick glance at the source of rpcbind() I think that there's no way
> that my_xrpt can be used uninitialized.  A proper fix for this warning
> would be then to just set my_xrpt to NULL at first and let the rest of the
> function unchanged regarding my_xrpt.

-- 
- Alfred Perlstein
- Research Engineering Development Inc.
- email: bright@mu.org cell: 408-480-4684
Comment 3 Giorgos Keramidas freebsd_committer 2004-09-12 23:25:11 UTC
On 2004-09-12 13:35, Alfred Perlstein <alfred@freebsd.org> wrote:
> I'm having a hard time seeing these warnings, how are they
> being triggered?

I tried building rpcbind with WARNS=3 or higher (up to 5).
Comment 4 Alfred Perlstein freebsd_committer 2004-09-12 23:38:33 UTC
* Giorgos Keramidas <keramida@freebsd.org> [040912 15:27] wrote:
> On 2004-09-12 13:35, Alfred Perlstein <alfred@freebsd.org> wrote:
> > I'm having a hard time seeing these warnings, how are they
> > being triggered?
> 
> I tried building rpcbind with WARNS=3 or higher (up to 5).

I don't see unitialized variables, I see bad casts.

-- 
- Alfred Perlstein
- Research Engineering Development Inc.
- email: bright@mu.org cell: 408-480-4684
Comment 5 Dan Lukes 2004-09-13 01:57:20 UTC
On Sun, 12 Sep 2004, Giorgos Keramidas wrote:

> He's probably the best person to suggest a fix for this warning, if one
> is really needed.

 	There is no need for fix a error as there are no error. The 'fd' is
correctly initialized with no exception, althought GCC can't evaluate it.

 	It's try to eliminate unnecesarry warning.

> No.  I don't know why you think that this is a good fix for all the
> uninitialized pointer warnings.  It's not.  Never :-/

 	Why ? It's written within the "description" section of the PR.
Warning shoult be "attention marks" - THIS CONSTRUCT SHOULD BE REVIEWED.

 	If there are too many warnings related to correct code we can't use
it for it purpose. Warnings become notice with almost no value.

 	Well. I compiled the code and evaluate that hundreds of warnings
didn't point to problematic code, but are "false" only. I will compile the
code next month. Do you think I recognise there are hundreds + 1 warning, so
we should evaulate this +1 warning to be sure there are no error ?

 	Warnings may be very usefull information, unless they lie so much.
Current code cause piles of false warnings. It "warnings" are hard to use as
true warning ...

 	But any commiter can close all of my PR if he think I'm not true ...

> After a quick glance at the source of rpcbind() I think that there's no way
> that my_xrpt can be used uninitialized.  A proper fix for this warning
> would be then to just set my_xrpt to NULL at first and let the rest of the
> function unchanged regarding my_xrpt.

 	The unnecesarry initialisation of variable initialised again later
seems to be vaste of resources. But IMHO only.
Comment 6 Dan Lukes 2004-09-13 02:19:35 UTC
On Sun, 12 Sep 2004, Alfred Perlstein wrote:

> I'm having a hard time seeing these warnings, how are they
> being triggered?

-Wall -O3

 	The "O3" is not for production environment, of course. But
evaluating of warnings can expose some of hidden bugs within code.

 	The inspection become to be very hard work unless we eliminate pile of
false warnings (despite the are not shown during "standard" compilation) ...

 						Dan
Comment 7 Giorgos Keramidas freebsd_committer 2004-09-13 06:43:38 UTC
On 2004-09-13 05:59, Dan Lukes <dan@obluda.cz> wrote:
> Dima Dorfman wrote:
> > Any initialization in the form "T v = v" invokes undefined behavior by
> > using the indeterminate value of an object. Eliminating a warning or
>
> 	Unless compiler documentation say other ...
> 	The v=v DURING DECLARATION (not later) is special case.
>
> 	Even on non GCC compiler it didn't make things worse - the value of v
> has not been changed by v=v statement ...

Undefined behavior is always worse.  It means you cannot determine what
the program will do until you run it, which is worse than setting a
pointer to NULL, which might be equivalent to:

	char *p = 0;

and translate to 1-2 machine instructions.

> > compiler can't be convinced that the variable is never used before
> > being initialized, please initialize it to something obviously bogus.
>
> 	It can be convicted, by "v=v" trick during declaration of variable.
> It's hack, of course.

A dangerous one too.  See my post about a "trap" that might be generated
when you try to set a pointer's value to something completely bogus.

> 	It seems to be similar to "declared but newer used" warning. It is
> eliminated by compiler specific hack also ("__unused")- not by true
> usage of variable.

That's a different thing.  Not using a variable isn't really dangerous.
Using the value of a pointer whose initial value contains garbage *is*
dangerous.

> If you thing those patches can't be used (with or without corrections
> you recommended) then close the PR

I think a lot of the work in these patches is useful.  A few changes
here, a minor fix there and it'll be nice if they're committed and have
as many warnings fixed as possible.  This is my personal opinion though.

Keep up the good work ;-)
Comment 8 Giorgos Keramidas freebsd_committer 2004-09-13 06:49:07 UTC
On 2004-09-13 02:57, Dan Lukes <dan@obluda.cz> wrote:
>On Sun, 12 Sep 2004, Giorgos Keramidas wrote:
>> After a quick glance at the source of rpcbind() I think that there's
>> no way that my_xrpt can be used uninitialized.  A proper fix for this
>> warning would be then to just set my_xrpt to NULL at first and let
>> the rest of the function unchanged regarding my_xrpt.
>
> 	The unnecesarry initialisation of variable initialised again later
> seems to be vaste of resources. But IMHO only.

True, but this is not the case here.  If you do initialize the value of
my_xrpt to something (i.e. to whatever value happens to be held in
my_xrpt at the time execution reaches the point of its declaration),
you should at least set it to something that is Right(TM).

This initialization

	char *foo = foo;

is not cheaper than this one:

	char *foo = NULL;

so if you're going to pay the penalty of initializing a variable to
shuttup the compiler, the second is preferrable because it explicitly
sets the pointer to a value that cannot be dereferenced.  This will
catch possible bugs (current or future) in the rest of the code, so
it's a lot safer.
Comment 9 Dan Lukes 2004-09-13 07:33:19 UTC
Giorgos Keramidas wrote:

> This initialization
> 	char *foo = foo;
> 
> is not cheaper than this one:
> 	char *foo = NULL;

	Not true. The first one is optimized out by GCC even if O level is 0.
Remember - it's hack/special construct (a special in-band way to tell
the compiler something special) - it's not real assignment.

	Check it by your favorite disassembler. I did it.

> This will catch possible bugs (current or future) in the rest of the code, so
> it's a lot safer.

	I agree with it.
					Dan


-- 
Dan Lukes,  SISAL, MFF UK  tel: +420 2 21914205, fax: +420 2 21914206
AKA: dan@obluda.cz, dan@freebsd.cz, dan@kolej.mff.cuni.cz, dan@fio.cz
Comment 10 Dan Lukes 2004-09-13 08:02:41 UTC
Giorgos Keramidas wrote:

> That's a different thing.  Not using a variable isn't really dangerous.
> Using the value of a pointer whose initial value contains garbage *is*
> dangerous.

	Unless you accidentally use "len" instead of "slen" which is totally
diferent variable. Even "declared but newer used" warning may warn
agains dangerous error within the code. It's the reason why any warning
should be carefully evaluated (it's the reason we should eliminate
warnings to already evaluated code)

> I think a lot of the work in these patches is useful.  A few changes
> here, a minor fix there and it'll be nice if they're committed and have
> as many warnings fixed as possible.  This is my personal opinion though.

	Well, I hope somebody commit it.

> Keep up the good work ;-)

	Thanks. And thanks for suggestions also.

Dan
Comment 11 Giorgos Keramidas freebsd_committer 2004-09-13 09:58:18 UTC
On 2004-09-13 06:46, Dima Dorfman <dd@freebsd.org> wrote:
> Dan Lukes <dan@obluda.cz> wrote:
> > Dima Dorfman wrote:
> > > Any initialization in the form "T v = v" invokes undefined behavior by
> > > using the indeterminate value of an object. Eliminating a warning or
> >
> > 	Unless compiler documentation say other ...
> > 	The v=v DURING DECLARATION (not later) is special case.
>
> I looked for such an exception in C99 before my original post, but I
> didn't find one.

There is a footnote in C99 that I'm a bit worried about.  In page 37, a
footnote reads:

        41) Thus, an automatic variable can be initialized to a trap
        representation without causing undefined behavior, but the value of
        the variable cannot be used until a proper value is stored in it.

Unless I'm mistaken, this means that the value of `foo' cannot be used in
the right part of the following assignment:

        char *foo = foo;

> Do I sound like a pedant yet? ;-)

For a while, I was afraid my comments would sound unnecessarily pedantic
too.  Fortunately, it seems I'm not.
Comment 12 Enji Cooper freebsd_committer 2015-11-10 11:12:07 UTC
This patch isn't right. Taking and will develop a fix.