Fix verious Coverity CIDS in rpc.lockd find_host()

[truckman]

Summary

Coverity reports the following problems in the find_host()
function in rpc.statd:

		1006368 Uninitialized pointer read
		1018506 Double free
		1305590 Resource leak
		1009293 Unchecked return value from library
		1194246 Wrong size argument

This patch addresses the first three problems by initializing
the ai1 and ai2 pointers to NULL at the top of the function,
and setting ai2 back to NULL after it is freed at the bottom
of the loop. Setting these to NULL when getaddrinfo() fails
should no longer be needed.

The last two problems are addressed by some tweaks to the
status file extend code.

Test Plan

Build and run

[hrs]

usr.sbin/rpc.statd/file.c

87 ↗

(On Diff #16401)

I think 1006368 is a false positive. getaddrinfo() stores a pointer to ai1 when it succeeds. The behavior is undefined when it fails. So the return code must be checked. Setting them to NULL in advance does not make sense.

96 ↗

(On Diff #16401)

This is the same with above. It is not guaranteed that ai2 will be set to NULL when getaddrinfo() fails.

[rmacklem]

This one is also a false positive, since ai2 is never used in the rest of the
function. However, if you want to shut the static analyzer up, this fix should
be harmless.

Btw, rpc.lockd and rpc.statd are not NFS and I didn't write them. Although I
don't really care, I don't see much point in "fixing" code that isn't buggy just
to shut some static analyser up.

[rmacklem]

Oh, and I agree with hrs@ that the code must check for the return
value of getaddrinfo().

ps: Sorry, I thought I was doing the last comment inline, but...

(I hate stupid web based...)

[rmacklem]

I tried to comment inline, but now its stuck in "save draft", so...
If hostname[0] is '\0', it is possible for ai2 to not be set NULL.
I'd suggest using the original "if"s but adding a ai2 = NULL;
just before the second one (a real fix if hostname[0] can be 0)
and ai1 = NULL; at the top just to shut the analyser up.

[truckman]

usr.sbin/rpc.statd/file.c

85 ↗

(On Diff #16401)

I didn't see any issues with ai1. I just added this to match ai2.

87 ↗

(On Diff #16401)

I don't think 1006368 is a false positive. if hp->hostname[0] == '\0', then the value of ai2 is whatever garbage was on the stack at function entry or possibly the value of ai2 from the previous loop iteration.

Our implementation of getaddrinfo() starts off with:

/* ensure we return NULL on errors */
 *res = NULL;

so clearing the pointer ahead of time isn't helpful.

For other implementations, there are two interesting cases of "undefined". One is that getaddrinfo() might not touch the pointer at all. In that case the pointer needs to either be cleared before or cleared afterwards on failure. The other is that getaddrinfo() might return a partial list on failure and just setting the pointer to NULL would cause a leak. The latter behavior would require setting the pointer to NULL before, and if the call fails and the pointer != NULL then calling freeaddrinfo() and clearing the pointer.

[truckman]

Partial revert to set ai* to NULL on getaddrinfo() failure.

[rmacklem]

The above patch works.
Since the "if" at line is in a loop and the freeaddrinfo(&ai2)
is also in the loop, I would have put the ai2 = NULL; between
line#94 and line#95 instead of at the top of the function and
at the end of the loop at #119.
However, this just seems clearer to me that setting it
before the loop and at the end of each iteration.

[truckman]

I placed ai2=NULL where I did to minimize the scope where ai2 had some value that was not a valid addrinfo list.