Page MenuHomeFreeBSD

Add missing permissions in open(2) calls with O_CREAT.
ClosedPublic

Authored by brooks on Jul 16 2019, 4:34 PM.

Details

Summary

The test suite contains a large number of instances of things like:

fd = open(path, O_RDWR | O_CREAT);

When O_CREAT is specified, the third, variadic argument is
required as the permission. If on is not passed, then depending
on the ABI, either the contents of the third argument register
or some arbitrary stuff on the stack will be used as the permission.
(On some out-of-tree ABIs, accessing nonexistant variadic arguments
leads to a fault and a failed test.)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooks created this revision.Jul 16 2019, 4:34 PM
asomers accepted this revision.Jul 16 2019, 5:23 PM

LGTM, though ngie sometimes objects to changes to these files because of the upstream divergence. It's a shame that neither the compiler nor Coverity caught this error. However, it's worth noting that Coverity _does_ catch 43 other errors in contrib/netbsd-tests; it might be worth proactively fixing those. BTW, what "out-of-tree ABIs" generate a fault?

This revision is now accepted and ready to land.Jul 16 2019, 5:23 PM

NetBSD just committed these changes and some others just now. I'm waiting for cvsweb to update so I can check what was actually committed.

In CheriABI (hardware fat pointers), va_list is a bounded array on the stack. In this case it ends up as a zero-length array so reading mode from it yields a fault rather then treating stack or register garbage as a mode.

ngie accepted this revision.EditedJul 16 2019, 10:15 PM

This is totally fine —- as long as the upstream commits are referenced, I’m ok with that (my concern is with changes not being communicated beforehand and having to reverse engineer where and how to upstream them to the other projects, since things tend to change from time to time).

It’s about time for me to pull down a new snapshot soon and reconcile the changes that have been made in the past couple years.

This revision was automatically updated to reflect the committed changes.