We need to check if "loader_str" is an actual path or not.

[BogdanAriton]

The string "loader_str" we got from split_load_options() is considered as a path but it could be other things, for example:
"S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}"
This is a Microsoft BCD object which represents the MS Boot loader:
BCDEDIT Friendly Name: {bootmgr}
Symbolic Name: GUID_WINDOWS_BOOTMGR
GUID: {9dea862c-5cdd-4e70-acc1-f32b344d4795}
This value is what normally the bootmgfw.efi file is expecting. The shim in some situations could replace bootmgfw.efi for various reasons.

We need to ignore such arguments and use the DEFAULT_LOADER.

Link to opened issue: #370

[BogdanAriton]

Thanks for reviewing the code. I've updated the white space issues.
Thanks!

[Metabolix]

You seem to assume that if the file does not exist, the path is incorrect. However, even if the path is just "loader.efi", your code says it's ”not a valid path” if it doesn't exist. In my opinion, this is not the way to go. While this approach luckily works for weird paths, it's incorrect and could cause confusion when a file is simply missing.

Also, you need to know what you are fixing and why your fix works. If you look at the boot entry which bcdedit.exe creates, you can see that the data probably contains binary data that you do not see on the screen. You can easily verify this by printing out the data in HEX in your function. For the record, your example "S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}" seems to be a valid path at least on OMVF (QEMU x64), so I could actually put my boot loader there. Also "䥗䑎坏S" (which is 8-bit "WINDOWS\0" interpreted as UCS-2) is a valid file name, even though you may only see the S if your system doesn't support all the symbols. What actually causes an error at least on my system is 8-bit "WINDOWS\0\x01\0" because the \x01 is not allowed in the file name.

[BogdanAriton]

Thank you so much for the response!
Of course some of the things are a tad confusing for me, so I've added my "misunderstandings" below:

You seem to assume that if the file does not exist, the path is incorrect. However, even if the path is just "loader.efi", your code says it's ”not a valid path” if it doesn't exist. In my opinion, this is not the way to go. While this approach luckily works for weird paths, it's incorrect and could cause confusion when a file is simply missing.

If the path is "loader.efi" it will be concatenated with the folder location where the shim resides. (done here: generate_path_from_image_path). - so we eventually check if /path/to/shim/loader.efi exists.
If the file is missing, what should the correct approach should be?

Also, you need to know what you are fixing and why your fix works. If you look at the boot entry which bcdedit.exe creates, you can see that the data probably contains binary data that you do not see on the screen. You can easily verify this by printing out the data in HEX in your function. For the record, your example "S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}" seems to be a valid path at least on OMVF (QEMU x64), so I could actually put my boot loader there. Also "䥗䑎坏S" (which is 8-bit "WINDOWS\0" interpreted as UCS-2) is a valid file name, even though you may only see the S if your system doesn't support all the symbols. What actually causes an error at least on my system is 8-bit "WINDOWS\0\x01\0" because the \x01 is not allowed in the file name.

Originally, in the message that the shim prints out, I was only able to see the S at the start which was concatenated with the rest of the path where the shim would reside (which is consistent with what you said about which characters can be seen on the screen based on the system). I printed out as you said the entire buffer and this is what I got. ("S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}" ) - but I'm confused about when you say that is seems to be a valid path? This is just a GUID that the windows boot loader recognizes, for the shim this doesn't mean anything since the shim is trying to load another file (a second stage loader or just keep the default loader). This is why I thought to code would be useful just checking for the files existence, because we can't make much sense of other options in the first place.

[Metabolix]

If the file is missing, what should the correct approach should be?

Currently shim reports ”Failed to open ...” and it's a perfectly good error message when the path is valid but the file is missing.

This is exactly why I think your idea needs a more complicated implementation than just checking if the file exists. If that was a proper solution, then you could simply rely on the check which shim already does and just hide the unwanted error message in shim.c. No need to add any duplicate code for that.

I printed out as you said the entire buffer and this is what I got. ("S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}")

That's not the entire buffer. The entire buffer is more like:

00000000  57 49 4e 44 4f 57 53 00  01 00 00 00 88 00 00 00  |WINDOWS.........|
00000010  78 00 00 00 42 00 43 00  44 00 4f 00 42 00 4a 00  |x...B.C.D.O.B.J.|
00000020  45 00 43 00 54 00 3d 00  7b 00 39 00 64 00 65 00  |E.C.T.=.{.9.d.e.|
00000030  61 00 38 00 36 00 32 00  63 00 2d 00 35 00 63 00  |a.8.6.2.c.-.5.c.|
00000040  64 00 64 00 2d 00 34 00  65 00 37 00 30 00 2d 00  |d.d.-.4.e.7.0.-.|
00000050  61 00 63 00 63 00 31 00  2d 00 66 00 33 00 32 00  |a.c.c.1.-.f.3.2.|
00000060  62 00 33 00 34 00 34 00  64 00 34 00 37 00 39 00  |b.3.4.4.d.4.7.9.|
00000070  35 00 7d 00 00 00 77 00  01 00 00 00 10 00 00 00  |5.}...w.........|
00000080  04 00 00 00 7f ff 04 00                           |........|
00000088

but I'm confused about when you say that is seems to be a valid path? This is just a GUID that the windows boot loader recognizes,

Even if it makes no sense, it's a valid path. You can make a file named "S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}", and you can tell shim to boot that file. But it doesn't really matter, I was really just pointing out that loader_str does not actually contain the string you reported.

[BogdanAriton]

If the file is missing, what should the correct approach should be?

Currently shim reports ”Failed to open ...” and it's a perfectly good error message when the path is valid but the file is missing.

This is exactly why I think your idea needs a more complicated implementation than just checking if the file exists. If that was a proper solution, then you could simply rely on the check which shim already does and just hide the unwanted error message in shim.c. No need to add any duplicate code for that.

I printed out as you said the entire buffer and this is what I got. ("S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}")

That's not the entire buffer. The entire buffer is more like:

00000000  57 49 4e 44 4f 57 53 00  01 00 00 00 88 00 00 00  |WINDOWS.........|
00000010  78 00 00 00 42 00 43 00  44 00 4f 00 42 00 4a 00  |x...B.C.D.O.B.J.|
00000020  45 00 43 00 54 00 3d 00  7b 00 39 00 64 00 65 00  |E.C.T.=.{.9.d.e.|
00000030  61 00 38 00 36 00 32 00  63 00 2d 00 35 00 63 00  |a.8.6.2.c.-.5.c.|
00000040  64 00 64 00 2d 00 34 00  65 00 37 00 30 00 2d 00  |d.d.-.4.e.7.0.-.|
00000050  61 00 63 00 63 00 31 00  2d 00 66 00 33 00 32 00  |a.c.c.1.-.f.3.2.|
00000060  62 00 33 00 34 00 34 00  64 00 34 00 37 00 39 00  |b.3.4.4.d.4.7.9.|
00000070  35 00 7d 00 00 00 77 00  01 00 00 00 10 00 00 00  |5.}...w.........|
00000080  04 00 00 00 7f ff 04 00                           |........|
00000088

but I'm confused about when you say that is seems to be a valid path? This is just a GUID that the windows boot loader recognizes,

Even if it makes no sense, it's a valid path. You can make a file named "S xBCDOBJECT={9dea862c-5cdd-4e70-acc1-f32b344d4795}", and you can tell shim to boot that file. But it doesn't really matter, I was really just pointing out that loader_str does not actually contain the string you reported.

Hmm, I understand what you're saying, but think about it this way:
The loader_str is the result of parsing whatever data you get as a load option (and this can be valid or not) it will then replace the second_stage value, but when you build shim with a default loader value you would want to use that instead of an incorrect path. That's the reason for the path test. If the load option is correct, then use that, if not keep the default value.

I my mind it doesn't make sense to get some value from the load options which can be something wrong only to then use the fallback. The path check is just something that I believe it will be useful overall and it's agnostic of whether it's a windows specific load option or something else. Maybe it can be implemented in a different way and we can give back some more meaningful messages, but in my head it make sense since we're trying to load up a file and if not found just go for the DEFAULT value or use the fallback if that doesn't exists.

[Metabolix]

If the load option is correct, then use that, if not keep the default value.

That already happens. The necessary checks are already in place (when shim tries to load the file), and now you are just duplicating them in another place. The functional difference is in output. I think you should just quiet the offending error message instead of duplicating all the checks. That's a smaller change and makes the intention clear.

However, I see the goal differently: if the user creates a boot entry with a new path, that's what they want to use, and they want to know if it's not working. The user should have control over their boot options, and only special cases (such as bcdedit) prevent it. So instead of ignoring all errors, we should ignore only those where the user is clearly not in control.

Let a mainainer say which option is preferable.

[BogdanAriton]

If the load option is correct, then use that, if not keep the default value.

That already happens. The necessary checks are already in place (when shim tries to load the file), and now you are just duplicating them in another place. The functional difference is in output. I think you should just quiet the offending error message instead of duplicating all the checks. That's a smaller change and makes the intention clear.

However, I see the goal differently: if the user creates a boot entry with a new path, that's what they want to use, and they want to know if it's not working. The user should have control over their boot options, and only special cases (such as bcdedit) prevent it. So instead of ignoring all errors, we should ignore only those where the user is clearly not in control.

Let a mainainer say which option is preferable.

OK, I'm fine with closing this PR out and just check for WINDOWS\0 case added by bcedit. This is probably the only case, thus far that causes issues which most people don't understand. (like the ones described in the ticket)