Bug 239315 - Forth loader misreads nextboot.conf files over 80 bytes
Summary: Forth loader misreads nextboot.conf files over 80 bytes
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Warner Losh
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-07-19 19:22 UTC by John Hood
Modified: 2023-09-17 14:25 UTC (History)
3 users (show)

See Also:


Attachments
one-line fix (334 bytes, patch)
2019-07-19 19:22 UTC, John Hood
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Hood 2019-07-19 19:22:09 UTC
Created attachment 205904 [details]
one-line fix

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.
Comment 1 commit-hook freebsd_committer freebsd_triage 2021-07-11 14:48:14 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9c1c02093b90ae49745a174eb26ea85dd1990eec

commit 9c1c02093b90ae49745a174eb26ea85dd1990eec
Author:     John Hood <cgull+l-freebsd-bugzilla@glup.org>
AuthorDate: 2021-07-11 14:44:12 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-11 14:47:29 +0000

    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

 stand/forth/support.4th | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 2 Warner Losh freebsd_committer freebsd_triage 2021-07-11 14:50:00 UTC
Thanks! This is clearly bad. Sorry for the delay in getting it into the tree. Pushed to main, and queued in my MFC branches.
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-07-16 18:30:57 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=37053c21e9d984e50d7cac0002f8ec370213c7b1

commit 37053c21e9d984e50d7cac0002f8ec370213c7b1
Author:     John Hood <cgull+l-freebsd-bugzilla@glup.org>
AuthorDate: 2021-07-11 14:44:12 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-16 18:28:44 +0000

    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 9c1c02093b90ae49745a174eb26ea85dd1990eec)

 stand/forth/support.4th | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2021-07-21 16:19:32 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=47872383e140fb705222399ad6f7a898286b1bc2

commit 47872383e140fb705222399ad6f7a898286b1bc2
Author:     John Hood <cgull+l-freebsd-bugzilla@glup.org>
AuthorDate: 2021-07-11 14:44:12 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-21 16:16:31 +0000

    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 9c1c02093b90ae49745a174eb26ea85dd1990eec)

 stand/forth/support.4th | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 5 Warner Losh freebsd_committer freebsd_triage 2021-07-26 22:43:02 UTC
This broke booting with old nextboot.conf files. I had to revert 9c1c02093b90ae49745a174eb26ea85dd1990eec in 4783fb730fa1cfdbe5c905bb23ac74f681e2df6b.
Comment 6 commit-hook freebsd_committer freebsd_triage 2021-07-27 02:38:48 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=320e5d7d398c9b66e73560e7789e39709d31ce7d

commit 320e5d7d398c9b66e73560e7789e39709d31ce7d
Author:     Warner Losh <imp@FreeBSD.org>
AuthorDate: 2021-07-26 22:40:41 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-27 02:37:40 +0000

    Revert "loader: support.4th resets the read buffer incorrectly"

    This reverts commit 9c1c02093b90ae49745a174eb26ea85dd1990eec. It seems
    to have broken all old nextboot.conf files causing hangs on boot.

    PR:     239315

    (cherry picked from commit 4783fb730fa1cfdbe5c905bb23ac74f681e2df6b)

 stand/forth/support.4th | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2021-07-27 02:45:51 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ac56cd3ba627b403da2c52ae6cecb5b4143be6c7

commit ac56cd3ba627b403da2c52ae6cecb5b4143be6c7
Author:     Warner Losh <imp@FreeBSD.org>
AuthorDate: 2021-07-26 22:40:41 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-27 02:38:58 +0000

    Revert "loader: support.4th resets the read buffer incorrectly"

    This reverts commit 9c1c02093b90ae49745a174eb26ea85dd1990eec. It seems
    to have broken all old nextboot.conf files causing hangs on boot.

    PR:             239315
    Sponsored by:   Netflix

    (cherry picked from commit 4783fb730fa1cfdbe5c905bb23ac74f681e2df6b)

 stand/forth/support.4th | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 8 cgull 2021-07-27 19:06:39 UTC
Ouch.  How did the older nextboot.conf fail, and what did they contain or is there an example?
Comment 9 Warner Losh freebsd_committer freebsd_triage 2021-07-27 19:39:22 UTC
(In reply to cgull from comment #8)
The loader hung before jumping to the kernel.
I'll see if I can get a copy of the offending file.

A large amount of the benefit from this fix, though, can be had by bumping 80 to 200....

I'd like a better explanation of why the one-line fix should work. It seems to me resetting the pointer to 0 is the right thing, so I'd like to understand why setting the read_buffer len to 0 is right.
Comment 10 cgull 2021-07-28 02:12:55 UTC
I had to study the Forth code again, I'd forgotten the details of the bug.  I think I've gotten the explanation right here.

`read_buffer` is a string structure, as defined in support.4th.  It's an addr/len pair.

`read_buffer_ptr` is an index into the string (not a pointer).  Its value can be 0 to `read_buffer_size`.

When the command in comment #1 is run on a system that boots from UFS, it writes /boot/nextboot.conf with the contents:

nextboot_enable="YES"
foo="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"

On ZFS boot, it only writes:

foo="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"

My original repro was done on UFS.  It looks like on ZFS the loader doesn't use nextboot.conf at all.

The `start` word in loader.4th first calls `initialize` in loader.4th.  This calls `include_nextboot_file`.   That function keeps state in the `nextboot_enable` environment variable and `nextboot?` Forth variable.  The first time it's called, the variable isn't set, and it will call `peek_file`, which reads the first line of nextboot.conf and sets an environment variable (so the first line had _better_ be `nextboot_enable=...`)  In this case, `include_nextboot_file` returns without doing anything else.  `initialize` then calls `include_conf_files` which starts loading /boot/loader.conf and so on.

When /boot/nextboot.conf is >80 bytes and the first line is shorter than that (always, given the requirement that `nextboot_enable` is first), peek_file reads the first 80 bytes into `read_buffer`.  `end_of_file?` is not set to true.  At this point, read_buffer contains something like

nextboot_enable="YES"
foo="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

(with no trailing linefeed), and `read_buffer_ptr` points just past the first line.

When `read_conf_files` is called, eventually it gets to opening the first file (usually loader.conf), `load_conf` is called, it calls `reset_line_reading`, which resets `read_buffer_ptr` but not `read_buffer`.  It then calls `process_conf` -> `read_line` -> `skip_newlines`/`scan_buffer` which happily read stale characters from `read_buffer`, until it is exhausted and they have to read from the input file again.  Since `read_buffer` probably ended with a truncated line, those characters are inserted before the first actual line from loader.conf.

When /boot/nextboot.conf is <= 80 bytes long, all this still happens.  But since `read_buffer` contains valid, non-truncated lines, any remaining lines in the buffer from nextboot.conf are parsed and evaluated before loader.conf is actually read, idempotently.  `initialize` later calls `include_nextboot_file` to give it the last say on setting variables, all the nextboot.conf variables get read again, and everything works as expected, more or less.
Comment 11 cgull 2021-07-28 03:22:44 UTC
What is (was) committed in the repo isn't what I submitted: my change adds a line, but current and MFCs all replace a line.  Both are needed.

It's also wrong on Phabricator and I missed it there-- apologies...
Comment 12 Warner Losh freebsd_committer freebsd_triage 2021-07-28 05:09:22 UTC
Yea, I botched the original application. My apologies. I've sent the corrected version to the regression reporter for final sanity testing.

which phab review? I don't see one in my open reviews and didn't have one in the commit message for auto-close :(

I just created one https://reviews.freebsd.org/D31328 to make sure this time.
Comment 13 cgull 2021-07-28 15:40:02 UTC
I had received an email notification for https://reviews.freebsd.org/R10:37053c21e9d984e50d7cac0002f8ec370213c7b1 and reviewed that.  It's on the stable/13 commit, so I'm guessing it's auto-generated.

I've also reviewed D31328 now.
Comment 14 commit-hook freebsd_committer freebsd_triage 2021-07-28 19:55:10 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=dbdf2b52f59df7374eb1f799b4df1b54e4502e40

commit dbdf2b52f59df7374eb1f799b4df1b54e4502e40
Author:     John Hood <cgull+l-freebsd-bugzilla@glup.org>
AuthorDate: 2021-07-28 19:43:02 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-28 19:50:38 +0000

    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.

    Tested by:              Gary Jennejohn
    PR:                     239315
    MFC After:              3 weeks
    Reviewed by:            imp (and correctly applied this time)
    Differential Revision:  https://reviews.freebsd.org/D31328

 stand/forth/support.4th | 1 +
 1 file changed, 1 insertion(+)
Comment 15 commit-hook freebsd_committer freebsd_triage 2021-09-12 16:36:35 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=3a4b9e30d4119b952fd0690cb7ab8eb9c4346317

commit 3a4b9e30d4119b952fd0690cb7ab8eb9c4346317
Author:     John Hood <cgull+l-freebsd-bugzilla@glup.org>
AuthorDate: 2021-07-28 19:43:02 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-09-12 15:56:15 +0000

    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.

    Tested by:              Gary Jennejohn
    PR:                     239315
    MFC After:              3 weeks
    Reviewed by:            imp (and correctly applied this time)
    Differential Revision:  https://reviews.freebsd.org/D31328

    (cherry picked from commit dbdf2b52f59df7374eb1f799b4df1b54e4502e40)

 stand/forth/support.4th | 1 +
 1 file changed, 1 insertion(+)
Comment 16 commit-hook freebsd_committer freebsd_triage 2021-09-12 18:32:54 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=caf5824ff203bd8d295e0df538d1db99ee415d93

commit caf5824ff203bd8d295e0df538d1db99ee415d93
Author:     John Hood <cgull+l-freebsd-bugzilla@glup.org>
AuthorDate: 2021-07-28 19:43:02 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-09-12 16:33:13 +0000

    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.

    Tested by:              Gary Jennejohn
    PR:                     239315
    MFC After:              3 weeks
    Reviewed by:            imp (and correctly applied this time)
    Differential Revision:  https://reviews.freebsd.org/D31328

    (cherry picked from commit dbdf2b52f59df7374eb1f799b4df1b54e4502e40)

 stand/forth/support.4th | 1 +
 1 file changed, 1 insertion(+)