Fix bootchooser for the output of efibootmgr 18

[dvzrv]

Since efibootmgr 18, the default output of efibootmgr is now more verbose [1], which breaks the assumptions made by RAUC in regards to parsing the output. This issue is also affecting others [2].

Fix the parsing logic by unconditionally splitting on a tab in the detected name part of the boot entry, so that the unused data part can be discarded.
Emit a debug message for each found boot entry, as this helps in detecting issues in the future.

[1] rhboot/efibootmgr@8ec3e9d
[2] rhboot/efibootmgr#169

This fix is not related to an open ticket, as I noticed this quite recently and got in touch on matrix to discuss a possible fix.
I verified this to work in an Arch Linux VM using efibootmgr 18 and a RAUC 1.10 with this patch applied.

As writing C is not really my forte, I'd appreciate feedback (especially whether this works as intended with efibootmgr < 18).

[jluebbe]

To make sure this can work reliably, we need to check that the boot name doesn't contain whitespace. See https://github.com/rauc/rauc/blob/master/src/config_file.c#L235 for the place where it is read. That area also has examples for reporting an error. The bootname check should be in a helper function.

[dvzrv]

To make sure this can work reliably, we need to check that the boot name doesn't contain whitespace. See https://github.com/rauc/rauc/blob/master/src/config_file.c#L235 for the place where it is read. That area also has examples for reporting an error. The bootname check should be in a helper function.

Maybe I lack context here, but why can it not contain a whitespace? This seems certainly possible in the context of e.g. efibootmgr.

[jluebbe]

To make sure this can work reliably, we need to check that the boot name doesn't contain whitespace. See https://github.com/rauc/rauc/blob/master/src/config_file.c#L235 for the place where it is read. That area also has examples for reporting an error. The bootname check should be in a helper function.

Maybe I lack context here, but why can it not contain a whitespace? This seems certainly possible in the context of e.g. efibootmgr.

We currently don't have checks for the bootname, so it could contain a tab character. In that case, applying this change would break at runtime. Adding a check during config parsing would report that issue early. And while adding that check, we should also reject any unusual bootnames containing whitespace (to be better prepared for future efibootmgr changes).

[dvzrv]

We currently don't have checks for the bootname, so it could contain a tab character. In that case, applying this change would break at runtime. Adding a check during config parsing would report that issue early. And while adding that check, we should also reject any unusual bootnames containing whitespace (to be better prepared for future efibootmgr changes).

I see. But validating the config input and more robustly dealing with the output of efibootmgr are two separate issues (in my mind).
Are there any straightforward facilities to search for the right-most tab character? Maybe that would make more sense.

[dvzrv]

Are there any straightforward facilities to search for the right-most tab character? Maybe that would make more sense.

Ah, nevermind. That'd of course be problematic with efibootmgr < 18 and a bootname containing a single tab.

So generally you'd rather not allow tabs or spaces then, which makes sense I guess.

[codecov]

Codecov Report

Patch coverage: 96.55% and project coverage change: +0.02% 🎉

Comparison is base (966a86b) 80.28% compared to head (84ecf94) 80.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   80.28%   80.30%   +0.02%     
==========================================
  Files          58       58              
  Lines       18000    18029      +29     
==========================================
+ Hits        14451    14479      +28     
- Misses       3549     3550       +1     

Files Changed	Coverage Δ
include/utils.h	100.00% <ø> (ø)
src/bootchooser.c	79.37% <80.00%> (+<0.01%)	⬆️
src/config_file.c	86.41% <100.00%> (+0.07%)	⬆️
src/utils.c	76.29% <100.00%> (+0.51%)	⬆️
test/config_file.c	99.85% <100.00%> (+<0.01%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jluebbe]

I've adjusted the commit titles to our usual format, made some changes to the code (see the fixup commits) and added a test-case for the config value check. @dvzrv does this look OK for you? Could you test it with efibootmgr 18?

[dvzrv]

I've adjusted the commit titles to our usual format, made some changes to the code (see the fixup commits) and added a test-case for the config value check. @dvzrv does this look OK for you? Could you test it with efibootmgr 18?

Thanks! I've rebuild the package for Arch Linux locally with the updated fixes. Tests run fine and after installing the updated package in a VM (with A/B setup) the output of rauc status --detailed works without issue.

[ejoerns]

As we already have a regex there, wouldn't it make more sense to adapt this?

--- a/src/bootchooser.c
+++ b/src/bootchooser.c
@@ -1147,7 +1147,7 @@ static gboolean efi_bootorder_get(GList **bootorder_entries, GList **all_entries
        }

        /* Obtain mapping of efi boot numbers to bootnames */
-       regex = g_regex_new("^Boot([0-9a-fA-F]{4})[\\* ] (.+)$", G_REGEX_MULTILINE, 0, NULL);
+       regex = g_regex_new("^Boot([0-9a-fA-F]{4})[\\* ] ([^\\t\\n]+).*$", G_REGEX_MULTILINE, 0, NULL);
        if (!g_regex_match(regex, g_bytes_get_data(stdout_buf, NULL), 0, &match)) {
                g_set_error(
                                error,

works v17 and v18 this seems to work for me.

I have also an example here where there is a space in an EFI boot entry. Thus I would assume having spaces is uncommon but possible. Why would we want to reject both tabs and spaces if only tabs collide with known assumptions?

[jluebbe]

Spaces/tabs are also problematic for bootnames in other places, like the kernel cmdline, so it's better to reject them early.

The strchr approach seems simpler, so I prefer it for the point release.

[ejoerns]

Spaces/tabs are also problematic for bootnames in other places, like the kernel cmdline, so it's better to reject them early.

Since a broken config will fail early with this approach, go for it.

The strchr approach seems simpler, so I prefer it for the point release.

Agreed for the point release.