loader: support.4th resets the read buffer incorrectly

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
Reviewed by: imp

(cherry picked from commit 9c1c02093b)
This commit is contained in:
John Hood 2021-07-11 08:44:12 -06:00 committed by Warner Losh
parent 9148a80a65
commit 37053c21e9

View File

@ -485,7 +485,7 @@ variable fd
get-current ( -- wid ) previous definitions >search ( wid -- )
: reset_line_reading
0 to read_buffer_ptr
0 read_buffer .len !
;
: read_line