Set f_file to -1/F_UNUSED when possible after closing it to avoid trashing/double-closing file descriptors later

[ngie]

Summary

Set f_file to -1/F_UNUSED when possible after closing them

This will help avoid trashing file descriptors that get reused later in the
daemon

Sponsored by: EMC / Isilon Storage Division
Submitted by: Miles Ohlrich <[e-mail]>

[ngie]

Remove diff with the Makefile

[ed]

Hi NGie,

Looking at the code, I see that in some places we discard an entry from the list by setting its type to F_UNUSED, instead of clearing the file descriptor number. Would you by any chance know what's up with that? Maybe all of these code paths should do the same? Still, I think that setting the file descriptor to -1 is a good idea to improve robustness.

If it's the case that setting the type to F_UNUSED would also need to be done, would it make sense to add a function, say, killfile(), that does both these things and use that throughout the code?

[markj]

In some places files are discarded by setting f->f_type to F_UNUSED. It seems like that's a more correct way to handle closed files, and I think the real bug is that we're not doing that everywhere.

At the very least, this should be lifted into a subroutine.

[ngie]

In D3081#60927, @markj wrote:

In some places files are discarded by setting f->f_type to F_UNUSED. It seems like that's a more correct way to handle closed files, and I think the real bug is that we're not doing that everywhere.

At the very least, this should be lifted into a subroutine.

Agreed. This should be put in a separate/static function.

Here's what Miles said he in an internal email:

"""
I will try to set up a Phabriactor account sometime soon so that I can comment, but here is what I would say:

fprintlog() will close the file descriptor (and not set it to -1), which means that the caller cannot determine if the file descriptor was closed. Coverity analysis found that logmsg() was calling fprintlog() and then calling close(f->f_file), which could cause a double close in some cases.
"""

So my interpretation was a bit off, but (in theory) if the file descriptor was reused, it could also cause problems.

[ngie]

In D3081#60926, @ed wrote:

Hi NGie,

Looking at the code, I see that in some places we discard an entry from the list by setting its type to F_UNUSED, instead of clearing the file descriptor number. Would you by any chance know what's up with that? Maybe all of these code paths should do the same? Still, I think that setting the file descriptor to -1 is a good idea to improve robustness.

If it's the case that setting the type to F_UNUSED would also need to be done, would it make sense to add a function, say, killfile(), that does both these things and use that throughout the code?

Something like that would be a good idea, yes (and I think that's what Mark was implying too).

[ngie]

• Move close/invalidation logic to close_filed
• Save/restore errno on entry/exit of close_filed

[ngie]

usr.sbin/syslogd/syslogd.c

357

I'm a bit torn about whether this should test f_file == -1 and return if it's true. The only thing is that it's a waste closing out an invalid file descriptor, i.e. it saves time having to context switch into the kernel, but close(-1) should be relatively quick...

362

Not sure if I should re-add (void) for the unchecked close... Yay or nay?

[markj]

usr.sbin/syslogd/syslogd.c

357

Could you make it an error to close a filed of type F_UNUSED or with f_file < 0?

362

Maybe pass down the return value of close(2) here and make the caller decide whether or not to ignore it?

Though if you do that, it doesn't really make sense to suppress errno. But since most callers don't need to preserve errno (and there's one caller that still does it below), it seems a bit unnecessary to do that in the first place.

1039–1040

This file comes from a global constant called consfile which is initialized once, during startup. It doesn't seem correct to reset its type to F_UNUSED here, since consfile may get opened and closed multiple times.

[ngie]

usr.sbin/syslogd/syslogd.c

357

I could, but that might hurt more than help. syslogd is one of those things that we need to always work, no matter what.

362

All the current callers ignore it though. I think re-adding the (void) would be ok; if it we need to pass back the error, we can do this later.

1039–1040

Hmmm... this is an interesting case indeed.

I think that closing the file descriptor/setting it to -1 makes sense, but I agree that setting it to F_UNUSED might break some functionality, especially if consfile is used for initializing the file descriptor again via the signal handler.

I'll put this back the way it was before -- thanks :)!

[ed]

usr.sbin/syslogd/syslogd.c

357

This test isn't needed, right? It doesn't seem to be any invocation of it where f could potentially be NULL.

360

Instead of saving and restoring errno, you could also just write:

if (f->f_file >= 0) {
    close(f->f_file);
    f->f_file = -1;
}

As long as you don't close a descriptor twice, errno shouldn't get clobbered. Calling close() with bad file descriptor numbers will only clutter truss output.

[cem]

usr.sbin/syslogd/syslogd.c

360

VOP_CLOSE errors (think NFS) may also propagate back to close(2).

[ngie]

usr.sbin/syslogd/syslogd.c

357

It's defensive programming.. but not needed based on code inspection. Still, syslogd crashing because of bad pointer handling seems really bad.

360

It simplifies the code to just check for the file descriptor being -1, then returning.

I looked at some of the code that didn't save/restore error, and it set the errno value explicitly to avoid logging the strerror message in log_deadchild, etc.

[ngie]

• Don't mark consfile F_UNUSED
• Look for f->f_file == -1, and return early to simplify close_filed
• Restore errno save/restore to just the previously affected areas as other areas affected by the new function mangle errno by design

[ngie]

Ping?