getrusage(2): fix return value under 32-bit emulation

[asomers]

Summary

getrusage(2): fix return value under 32-bit emulation

According to the man page, getrusage(2) should return EFAULT if the rusage
argument lies outside of the process's address space. But due to an oversight
in r100384, that's never been the case during 32-bit emulation. Fix it.

PR: 230153
Reported-by: tests(7)

Test Plan

Run lib/libc/sys/getrusage_test:getrusage_err under 32-bit emulation

[kib]

head/sys/compat/freebsd32/freebsd32_misc.c

886

If error != 0 there, calling freebsd32_rusage_out() does not make sense.

[asomers]

head/sys/compat/freebsd32/freebsd32_misc.c

886

It won't be useful, but it won't be harmful, either. It's just copying between two structures on the stack. I thought it was simpler this way. Would you like me to change it?

[kib]

head/sys/compat/freebsd32/freebsd32_misc.c

886

IMO it would be cleaner. Also, do not underestimate the evil stupidity of the modern compilers, where accessing non-initialized memory gives carte blanche to the undefined behaviour, e.g. 'int 3'. E.g. under LTO.

[jhb]

I'm confused by this change. Why did it not work before? Did the != NULL check not work so that it was never calling copyout? Otherwise, it was calling copyout correctly I don't see how EFAULT would not have been returned.

[asomers]

In D16500#350454, @jhb wrote:

I'm confused by this change. Why did it not work before? Did the != NULL check not work so that it was never calling copyout? Otherwise, it was calling copyout correctly I don't see how EFAULT would not have been returned.

The NULL check did work, and that was the problem. The ordinary version of this syscall doesn't have the NULL check, and according to the man page it shouldn't be there. With the NULL check in place, freebsd32_getrusage would return 0 when it should've returned EFAULT. copyout is responsible for generating the EFAULT errno.

[jhb]

In D16500#350455, @asomers wrote:

In D16500#350454, @jhb wrote:

I'm confused by this change. Why did it not work before? Did the != NULL check not work so that it was never calling copyout? Otherwise, it was calling copyout correctly I don't see how EFAULT would not have been returned.

The NULL check did work, and that was the problem. The ordinary version of this syscall doesn't have the NULL check, and according to the man page it shouldn't be there. With the NULL check in place, freebsd32_getrusage would return 0 when it should've returned EFAULT. copyout is responsible for generating the EFAULT errno.

Ah. I think I would perhaps have left the previous code flow (early return) as kib@ had noted mostly because it would have made the actual change (removing the NULL check) more obvious, e.g. if the diff had been:

-    if (uap->usage != NULL) {
-        freebsd32_rusage_out(...);
-        error = copyout(...);
-    }
+   freebsd32_rusage_out(...);
+   error = copyout(...);

Perhaps for future reference it might have been useful to explicitly say that you were removing the NULL check so that NULL failed with EFAULT. Other invalid values like (char *)1 should have failed, it was only NULL that didn't but the commit message to me read as if EFAULT wasn't working at all which I think is why I was confused.