Page MenuHomeFreeBSD

Report error to user if image activator fails with ENOMEM
ClosedPublic

Authored by emaste on Jun 3 2015, 8:09 PM.

Details

Summary

Previously kern_exec silently failed, killing the process with SIGABRT.

Posted for discussion. My take is that ENOMEM is a useful error to report, but others may not have much value.

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

emaste updated this revision to Diff 5924.Jun 3 2015, 8:09 PM
emaste retitled this revision from to Report error to user if image activator fails with ENOMEM.
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added a reviewer: kib.
emaste set the repository for this revision to rS FreeBSD src repository.
kib edited edge metadata.Jun 3 2015, 8:28 PM

I would not object against addition of the uprintf(), possibly managed to silence with some sysctl. Look at the kern_exec.c disallow_high_osrel which might be covered by the same knob.

But I think that to be useful, we should add uprintf() to image activators error situations, and not to the summary error path in the kern_exec.c. I.e. I suggest to annotate with uprintf() imgact_elf.c. You would be able to provide much more precise and up to the point diagnostic there. Like, in your case, "text segment too large".

emaste added a comment.Jun 3 2015, 9:48 PM
In D2731#51705, @kib wrote:

But I think that to be useful, we should add uprintf() to image activators error situations

Ok, I agree that will provide a much better user-facing error, and will post a new patch shortly.

op added a subscriber: op.Jun 3 2015, 10:51 PM
emaste updated this revision to Diff 5944.Jun 4 2015, 3:35 PM
emaste edited edge metadata.
emaste removed rS FreeBSD src repository as the repository for this revision.

Move reporting into imgact_elf.

emaste added a subscriber: trasz.Jun 4 2015, 3:35 PM
kib added a comment.Jun 5 2015, 8:07 AM

This could be made much shorter if you create a local variable of type char *, initialized to NULL. In the 'then' blocks of if()s, the var is asssigned a message, nothing more. Then, the final if(var != NULL) {PROC_UNLOCK(); uprintf();return();} is added.

Also, there are several other places in the imgact_elf.c where similar messages could be added in load_file(), e.g. e_type check, check for headers living in the first page, alignment etc.

emaste updated this revision to Diff 5963.Jun 5 2015, 1:57 PM

Use error message string pointer as suggested by kib, and add reporting for the other failure cases

emaste added inline comments.Jun 5 2015, 2:01 PM
sys/kern/imgact_elf.c
912 ↗(On Diff #5963)

I could also combine RLIMIT_DATA and RACCT_DATA into a single "Data segment size exceeds limit" and similar for _VMEM. Having distinct messages allows for finer-grained troubleshooting steps, but perhaps it's not necessary.

kib added inline comments.Jun 6 2015, 11:07 PM
sys/kern/imgact_elf.c
758 ↗(On Diff #5963)

It is program headerS, at least I saw this usage most of the time, I believe.

780 ↗(On Diff #5963)

This is not correct, I think. This is not about interpreter, but about interpreter header, pointing to the file name to use. Might be, 'Invalid PT_INTERP' is enough ?

804 ↗(On Diff #5963)

'Cannot execute shared object'

emaste updated this revision to Diff 6004.Jun 7 2015, 1:09 AM

Improve error messages based on kib's feedback

emaste added a comment.Jun 7 2015, 1:51 AM
kib accepted this revision.Jun 7 2015, 1:59 PM
kib edited edge metadata.
kib added inline comments.
sys/kern/imgact_elf.c
735 ↗(On Diff #6004)

Could you do a follow-up, after this patch is committed, to clear the declaration blocks in the imgact_elf.c ? Initializations should go into the code blocks; vars should be reordered, pointers first, primitive arithmetic types afterward.

This revision is now accepted and ready to land.Jun 7 2015, 1:59 PM
This revision was automatically updated to reflect the committed changes.