Improve error handling by shym · Pull Request #613 · Solo5/solo5

shym · 2025-10-16T12:31:23Z

This PR replaces a couple of asserts with a proper, or at least better, handling of the corresponding error. In particular, it replaces asserts that check conditions that can, or maybe could, be controlled by the unikernel, either in its file format (hardening the ELF parser) or in the hypercalls it triggers. It also takes the opportunity to report (unspecified) error to the unikernels when an I/O operation failed on the host.

This is addressing #612, hopefully.

edwintorok · 2025-10-16T14:07:18Z

Fixes my testcase, thanks:

solo5-hvt: /var/home/edwin/git/solo5/out/default/crashes/id:000000,sig:06,src:000002,time:52084,execs:9138,op:int8,pos:24580,val:+1: Invalid or unsupported executable

hannesm

looks good to me

tenders/common/mft.c

tenders/hvt/hvt_module_blk.c

reynir · 2025-10-17T10:41:36Z

tenders/hvt/hvt_module_blk.c

+    if (ret != wr->len) {
+        /* Report that it failed somehow */
+        wr->ret = SOLO5_R_EUNSPEC;
+        return;
+    }


This changes the semantics so the unikernel can continue if the write fails. Which is perhaps okay. It's hard for the unikernel to act on it, though(?!)

From what I can read from man -s2 pwrite write it can return less if there is not enough disk space. Considering the file is checked to have a fixed size from the beginning I can only imagine this happening if the file is a) sparse, or b) has been truncated(!)

It’s always unclear whether there’s anything useful to do when things are starting to fail when they really shouldn’t, indeed.

It is not entirely clear to me whether it is worth to handle this situation gracefully. What is wrong about assertion failure and the unikernel exiting?

Handling this properly from the unikernel side may require some further information, i.e. transport the data from errno from the tender to the unikernel, and then deciding what to do in the unikernel.

Indeed I'd leave it as assertion failure for now, and if there's a convincing case how to handle such a pwrite error, we need to read errno and transport it to the unikernel so it can decide what to do. The same for the block read, and also network read and write.

The assertion failure can create a coredump on the host, which allows a unikernel to consume (potentially unbounded) disk space on the host. This could get especially bad if due to a bug it'd immediately crash again in the same place after a restart.

I think it'd be fine if the unikernel (tender) exited with a fatal error in this case, as long as it doesn't actually crash with a coredump, e.g. it could print an error message and exit with a specific exitcode.

👍🏾 sounds good to me to (f)printf a message and exit(1).

I updated the PR accordingly.

Thanks, I only put a minor comment to use strerror()

reynir · 2025-10-17T10:42:47Z

tenders/hvt/hvt_module_blk.c

+    if (ret != rd->len) {
+        /* Report that it failed somehow */
+        rd->ret = SOLO5_R_EUNSPEC;
+        return;
+    }


I think this can only happen if the file was truncated(?)

tenders/hvt/hvt_module_blk.c

hannesm · 2025-10-23T09:50:17Z

Would you mind to rebase and squash, so the CI has a chance to run this PR? Also, is there a very small ELF binary that you could add for testing?

edwintorok · 2025-10-23T10:02:11Z

Would you mind to rebase and squash, so the CI has a chance to run this PR? Also, is there a very small ELF binary that you could add for testing?

It is ~33KiB (attaching Zipped because github doesn't allow me to attach the binary directly)
bug.zip

hannesm · 2025-10-23T11:04:50Z

tenders/hvt/hvt_module_net.c

        return;
    }
-    assert(ret > 0);
+    if (ret <= 0) {


with #559 in mind, I think we can allow ret == 0?

let's keep that for a separate PR.

Oh! you’re right, that’s a bit silly. I’ll fix it.

let's leave it here as it was (to avoid changing semantics here), and discuss in #618 where @dinosaure has comments.

OK. (I fixed at least the silly message.)

Co-authored-by: Edwin Török <[email protected]>

Stop `assert`-ing conditions that could invalidated by specially crafted ELF files. Use sensible errors instead.

Avoid `assert`-ing a condition that is controlled by the unikernel

hannesm

CI is green as well. Test has been added.

shym · 2025-10-23T11:50:07Z

I added Edwin’s torture test in a first commit, to ensure that running the tests in that version yields:

not ok 76 buggyelf hvt
# (from function `hvt_run' in file tests.bats, line 103,
#  in test file tests.bats, line 704)
#   `hvt_run test_buggyelf/buggyelf.hvt' failed
# solo5-hvt: common/elf.c:479: elf_load_note: Assertion `note_size != 0 && note_size <= nhdr.h.n_descsz' failed.
# timeout: the monitored command dumped core

If an I/O operation failed on the host (whatever the reason), display an explicit message about it and exit normally instead of aborting on an assertion failure This hardens the hvt tender against unikernels which could try to make it fail an assertion (to try and create core dumps, if they are enabled)

dinosaure · 2025-10-27T14:39:15Z

Thanks for your work.

@omegametabroccolo

- Fix compilation on OpenBSD about `#ifdef` (@omegametabroccolo, @hannesm, @reynir, Solo5/solo5#614, related with Solo5/solo5#600) - Add GitHub actions to test Solo5 on different platforms (@hannesm, Solo5/solo5#616, Solo5/solo5#617 & Solo5/solo5#540) - Do not use `-Wstringopt-overflow` when we use `clang` for `test_ssp` (@hannesm, Solo5/solo5#607) - Show errors and exit instead of `assert false` (@shym, @edwintorok, @hannesm, @reynir, Solo5/solo5#613 & Solo5/solo5#612) - Define `.note.GNU-stack` when it's needed (@shym, @Kensan, @hannesm, @palainp, @dinosaure, Solo5/solo5#619, Solo5/solo5#570 & Solo5/solo5#562) - Detect MTU on TAP interface for hvt, spt and virtio (@reynir, @hannesm, @dinosaure, Solo5/solo5#605 & Solo5/solo5#606)

shym mentioned this pull request Oct 16, 2025

solo5-hvt ELF parser: assert can be trigerred by malformed ELF files #612

Closed

dinosaure approved these changes Oct 17, 2025

View reviewed changes

hannesm approved these changes Oct 17, 2025

View reviewed changes

reynir approved these changes Oct 17, 2025

View reviewed changes

shym force-pushed the cautious-asserts branch from 979bf67 to 026cafa Compare October 20, 2025 09:53

hannesm reviewed Oct 20, 2025

View reviewed changes

tenders/hvt/hvt_module_blk.c Outdated Show resolved Hide resolved

shym force-pushed the cautious-asserts branch 2 times, most recently from 96b86eb to 2214db4 Compare October 20, 2025 12:00

hannesm reviewed Oct 23, 2025

View reviewed changes

shym and others added 3 commits October 23, 2025 13:37

Add a buggy ELF to test the ELF parser

77b19ee

Co-authored-by: Edwin Török <[email protected]>

Harden the ELF parser

d61da35

Stop `assert`-ing conditions that could invalidated by specially crafted ELF files. Use sensible errors instead.

Return errors on very large I/O length

0073960

Avoid `assert`-ing a condition that is controlled by the unikernel

shym force-pushed the cautious-asserts branch from 2214db4 to 14b3884 Compare October 23, 2025 11:44

hannesm approved these changes Oct 23, 2025

View reviewed changes

shym force-pushed the cautious-asserts branch from 14b3884 to c2354b3 Compare October 23, 2025 11:59

shym force-pushed the cautious-asserts branch from c2354b3 to 1ffc06a Compare October 23, 2025 12:26

dinosaure approved these changes Oct 27, 2025

View reviewed changes

dinosaure merged commit 8a94384 into Solo5:main Oct 27, 2025
5 checks passed

dinosaure mentioned this pull request Oct 27, 2025

Release of Solo5.0.10.0 ocaml/opam-repository#28784

Merged

Conversation

shym commented Oct 16, 2025

Uh oh!

edwintorok commented Oct 16, 2025

Uh oh!

hannesm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hannesm commented Oct 23, 2025

Uh oh!

edwintorok commented Oct 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannesm left a comment

Choose a reason for hiding this comment

Uh oh!

shym commented Oct 23, 2025

Uh oh!

Uh oh!

dinosaure commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants