login_cap.c: Don't set errno to ERANGE on memory allocation failure

[olce]

Summary

Modified functions: login_getcaptime(), login_getcapnum(), login_getcapsize().

They all call cgetstr(), which returns -2 on such conditions and already sets
errno to ENOMEM, arguably the appropriate value for these functions as well.

No in-tree consumer currently checks for errno on error reported by these
functions, so this change has no other code impact.

Test Plan

Whole tree review and local tests done.

[andrew_tao173.riddles.org.uk]

It's perhaps worth noting that basically all of login_cap's memory handling is dubious - it leaks in several places, the manpages and the code disagree about whether values should be freed by the library or the caller, and it uses non-obvious and undocumented static state.

IMO, we should:

1. Change the documentation to conform to the other *BSDs, requiring the caller to free returned values;
2. Eliminate the static state (move into login_cap_t)
3. Fix remaining leaks in the code, e.g. in login_getcapnum

(why would anyone care, you ask? I want to be able to use login classes with an extended version of posix_spawn, which requires that the lookups be done in the parent process rather than in the child.)

[emaste]

It's perhaps worth noting that basically all of login_cap's memory handling is dubious ... IMO, we should:

That sounds reasonable to me, but I think it's sensible to commit this patch now, anyhow.

[olce]

In D40342#958959, @emaste wrote:

It's perhaps worth noting that basically all of login_cap's memory handling is dubious ... IMO, we should:

That sounds reasonable to me (...)

This indeed sounds reasonable as an improvement plan. This prompts me to share here a few rough thoughts on some profound problems of the current approach (which I think are independent, but maybe this deserves closer examination).

The static behavior has a benefit (if it's implemented everywhere; I should have said "had a benefit" because it's not the case currently): If there are no memory allocations then there are less points of failure in the process of trying to change identity/resource limits/etc. If failure is allowed to happen in preparation for effective operations (one that really modifies the process state, such as changing the environment, the UID, the umask, etc.), how do we deal with it? Do we distinguish between a failure of the effective operation and the fact that we just couldn't perform it because of a failure in preliminary steps? More generally, I think the current error reporting is generally bad, especially for the "big"/convenience functions that perform multiple changes. Consequently, there is no simple way (and arguably no way at all) to know for sure the real process state after a call (and which memory has been allocated, as @andrew_tao173.riddles.org.uk points out).

In the spirit of "tools, not policy" (where it makes sense; there might be places where how to handle failure is unequivocal), I think the different functions should provide a clear status on which operations succeeded, which one failed and, importantly, why. Big functions (such as setusercontext()) should not only report that but ideally also strive to minimize points of failure, e.g., by predicting and trying to allocate all memory needed for subsequent operations upfront, or by avoiding to run some system calls multiple times (such as in the case of user's personal configuration files).

To achieve all these goals, we may be tempted to just grow a new carefully-designed API and gradually convert consumers. There are not so many of them, but this may not be that easy. For example, I think some of them currently share the same code for all *BSDs, so that would mean unsharing code. That said, it's arguably desirable since behavior has diverged between the implementations, so treating them the same is bound to cause trouble at some point. The alternative is to fix the obvious mistakes and add more functions to get more insight on the actions (not) performed. This is doable but it won't be pretty and probably not a good idea long term.

So yes, I think there's quite a bit of work in this area...

In D40342#958959, @emaste wrote:

(...) I think it's sensible to commit this patch now, anyhow.

I agree, since I don't think it stands in the way of anything that has been evoked above.