[S] Change in flashrom[master]: cli_classic.c: Early init of layout obscures invalid memory access

[Angel Pons (Code Review)]

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Nikolai Artemiev.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67723 )

Change subject: cli_classic.c: Early init of layout obscures invalid memory access
......................................................................

Patch Set 4: Code-Review-1

(2 comments)

File cli_classic.c:

https://review.coreboot.org/c/flashrom/+/67723/comment/eeea74ca_25893b67 
PS2, Line 541: 	struct flashrom_layout *layout = NULL;
> The issue is not that NULL  means 'no layout' but rather it is the attempted use of a layout before perhaps there is one available. By early initialisation one obscures this compiler warning that a branch didn't explicitly handle the case 'no layout' before usage. Squelching a compiler warning that can indicate possibly issue with procedural control flow isn't a good idea which made the bug the previous commit in the chain go unnoticed.

Hmmm, I'm not sure if I follow. Did CB:67722 fix the problem so that this change would not cause a compiler warnerror?
> Ultimately `layout` can still end up as NULL if no layout provided/found, so anything that uses it (e.g. get_region_range call below) should check it is non-null.

I agree.
> But we can't really get the compiler to enforce that since it only checks the variable has been initialized, and it doesn't check for initialized-to-non-null.

I don't think we should rely on the (which?) compiler to warn about this. I'd
rather have explicit NULL checks.
> I'm ok with leaving layout set to NULL here instead of setting it to NULL in the new `else` block below.

I don't think we should have an else clause, as it's much harder to prove that the variable has been initialized.
> An alternative would be to delete `default_layout` in `struct flashctx` and assign `flash->layout` to a default layout in `probe_flash()`. Then users can override the default `flash->layout` value if they want, and flash->layout will always be a valid layout.

My main concern is that the cli_classic main function is extremely long, and with variables declared at the top of the file, one could accidentally use the layout before it's supposed to be initialized. I think there's a clean way to get around this: move the part of the code that initializes the layout into a function, so that one can declare the layout pointer as constant (`struct flashrom_layout *const layout = foo();`) and be sure that it has been initialized before it can be used. Currently, the layout from file is handled much earlier than layout from IFD or fmap, so there's cases when the layout pointer may be valid in only a few cases.
> Anyway I'm going to move this patch to the end since the other two are sufficient to fix the segfault.

Sounds good, thanks.

https://review.coreboot.org/c/flashrom/+/67723/comment/5011262d_d84568ac 
PS2, Line 1110: 		msg_gdbg("Valid layout could not be found without image.\n");
> Maybe we should change it to "No layout provided or found in image", it's really just for debugging.

I don't think you can print a correct message with this else clause. What happens if you provide the layout as a file (`-l foo.layout`)? I just noticed that its command-line parameter is processed much earlier than the other layout sources (IFD, fmap), so I would expect that this patch as-is breaks this functionality.

Update: I just tested it (and I found I broke stuff yesterday; CB:67752 fixes it) and indeed, if you try using a layout from a file and reach this code, the else branch will be taken even though a valid layout exists.

If you still want to print a message for debugging, you'd have to check if the layout pointer is NULL:

```
if (!layout)
    msg_gdbg("Not using a layout\n");
```