Page MenuHomeFreeBSD

loader: support.4th resets the read buffer incorrectly
ClosedPublic

Authored by imp on Jul 28 2021, 5:09 AM.

Details

Summary

Large nextboot.conf files (over 80 bytes) are not read correctly by the
Forth loader, causing file parsing to abort, and nextboot configuration
fails to apply.

Simple repro:

nextboot -e foo=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
shutdown -r now

That will cause the bug to cause a parse failure but shouldn't otherwise
affect the boot. Depending on your loader configuration, you may also
have to set beastie_disable and/or reduce the number of modules loaded
to see the error on a small console screen. 12.0 or CURRENT users will
also have to explicitly use the Forth loader instead of the Lua loader.
The error will look something like:

Warning: syntax error on file /boot/loader.conf.local
foo="xxxxxxxxxxxxxxnextboot_enable="YES"

^

/boot/support.4th has crude file I/O buffering, which uses a buffer
'read_buffer', defined to be 80 bytes by the 'read_buffer_size'
constant. The loader first tastes nextboot.conf, reading and parsing
the first line in it for nextboot_enable="YES". If this is true, then
it reopens the file and parses it like other loader .conf files.

Unfortunately, the file I/O buffering code does not fully reset the
buffer state in the reset_line_reading word. If the last file was read
to the end, that doesn't matter; the file buffer is treated as empty
anyway. But in the nextboot.conf case, the loader will not read to the
end of file if it is over 80 bytes, and the file buffer may be reused
when reading the next file. When the file is reread, the corrupt text
may cause file parsing to abort on bad syntax (if the corrupt line has
<>2 quotes in it), the wrong variable to be set, no variable to be set
at all, or (if the splice happens to land at a line ending) something
approximating normal operation.

The bug is very old, dating back to at least 2000 if not before, and is
still present in 12.0 and CURRENT r345863 (though it is now hidden by
the Lua loader by default).

Suggested one-line attached. This does change the behavior of the
reset_line_reading word, which is exported in the line-reading
dictionary (though the export is not documented in loader man pages).
But repo history shows it was probably exported for the PNP support
code, which was never included in the loader build, and was removed 5
months ago.

One thing that puzzles me: how has this bug gone unnoticed/unfixed for
nearly 2 decades? I find it hard to believe that nobody's tried to do
something interesting with nextboot, like load a kernel and filesystem,
which is what I'm doing.

PR: 239315

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp requested review of this revision.Jul 28 2021, 5:09 AM
cgull_glup.org requested changes to this revision.EditedJul 28 2021, 3:31 PM
cgull_glup.org added a subscriber: cgull_glup.org.

The end result of this diff is correct, but

  • Phabricator says it's based on a dead SVN repo, which is out of date on this file
  • simultaneously, the old state of the file seems to be that of the bad Git commit rather than the original code at 37053c21~ or the revert at 320e5d7d

So it can't be what you'll actually commit. Can you redo this review request in git based on current main? It is kind of nit-picking, but given the history here, let's make sure it goes correctly.

This revision now requires changes to proceed.Jul 28 2021, 3:31 PM

The end result of this diff is correct, but

  • Phabricator says it's based on a dead SVN repo, which is out of date on this file

Yea, nothing to be done about that...

  • simultaneously, the old state of the file seems to be that of the bad Git commit rather than the original code at 37053c21~ or the revert at 320e5d7d

Rebased.... my staging branch hadn't been updated...

So it can't be what you'll actually commit. Can you redo this review request in git based on current main? It is kind of nit-picking, but given the history here, let's make sure it goes correctly.

The svn legacy thing can't be helped, but I've rebased to tip of main, which will be what I commit.

LGTM-- though I don't understand exactly what the revisions are based on (your staging repo?) they all look good.

I also did some testing to make sure the change is working as expected, and some cases I hadn't tested before (>80 char lines, multiple lines), and it all looks good. I can only get the original bug to occur on a BIOS boot though, a GPT boot doesn't show the problem.

This revision is now accepted and ready to land.Jul 28 2021, 5:48 PM