Page MenuHomeFreeBSD

Report error to user if image activator fails with ENOMEM
ClosedPublic

Authored by emaste on Jun 3 2015, 8:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 7:26 PM
Unknown Object (File)
Thu, Dec 5, 8:10 AM
Unknown Object (File)
Sun, Dec 1, 11:46 PM
Unknown Object (File)
Nov 19 2024, 2:51 PM
Unknown Object (File)
Nov 17 2024, 2:20 PM
Unknown Object (File)
Nov 12 2024, 11:40 PM
Unknown Object (File)
Oct 29 2024, 10:17 AM
Unknown Object (File)
Oct 9 2024, 12:48 AM
Subscribers

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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 - subversion.

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".

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.

emaste edited edge metadata.
emaste removed rS FreeBSD src repository - subversion as the repository for this revision.

Move reporting into imgact_elf.

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.

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

sys/kern/imgact_elf.c
912

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.

sys/kern/imgact_elf.c
758

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

780

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

'Cannot execute shared object'

Improve error messages based on kib's feedback

kib edited edge metadata.
kib added inline comments.
sys/kern/imgact_elf.c
735

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.