Differential D19689

Factor out resource limit enforcement code in the ELF loader.
ClosedPublic
Actions

Authored by trasz on Mar 23 2019, 1:28 PM.

Details

Reviewers

Commits

rS345547: Factor out resource limit enforcement code in the ELF loader.

Summary

Factor out resource limit enforcement code in the ELF loader.
It makes the code slightly easier to follow, and might make
it easier to fix the resouce accounting to also account for
the interpreter.

The PROC_UNLOCK() is moved earlier - I don't see anything
it should protect; the lim_max() is a wrapper around lim_rlimit(),
and that, differently from lim_rlimit_proc(), doesn't require
the proc lock to be held.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

trasz created this revision.Mar 23 2019, 1:28 PM

Herald added a subscriber: imp. · View Herald TranscriptMar 23 2019, 1:28 PM

Harbormaster completed remote builds in B23263: Diff 55384.Mar 23 2019, 1:28 PM

kib added inline comments.Mar 23 2019, 1:52 PM

sys/kern/imgact_elf.c
826 ↗	(On Diff #55384)	Can these two lines be merged with 80-columns limit ?
833 ↗	(On Diff #55384)	merge the conditions from these two if()s.
851 ↗	(On Diff #55384)	I think it is useful to re-flow all moved comments
853 ↗	(On Diff #55384)	Make the flag check style-compliant: if ((phdr[i].p_flags & PF_X) != 0 && text_size < seg_size) {
1155 ↗	(On Diff #55384)	Why did you removed the lock ? This must be noted in the commit message together with explanation why it is safe.

trasz marked 3 inline comments as done.Mar 25 2019, 5:21 PM

trasz added inline comments.

sys/kern/imgact_elf.c
826 ↗	(On Diff #55384)	Not really, they overflow by one or two characters. Also, there seems to be an interesting distinction - the ones from the first line are initialized upfront, the ones from the second line are not.
851 ↗	(On Diff #55384)	Done. I hadn't touched the lower one, since reflowing it doesn't make it any shorter.
1155 ↗	(On Diff #55384)	From my understanding, the lock was there because of the call to lim_max() - but lim_max() is a wrapper around lim_rlimit(), which, differently from lim_rlimit_proc(), doesn't seem to require the proc lock. Right? And yes, you are right that I should mention this in the commit message; I'll update the description.