wxFileConfig asserts on a config line with an empty key name (Issue #26914)
alex9849
alex9849 created an issue (wxWidgets/wxWidgets#26914)Description
Bug description:
wxFileConfig accepts a config line whose key name is empty, for example a line that is just =1.
wxFileConfig::Parse hands the empty key to wxFileConfigGroup::AddEntry, and the wxFileConfigEntry constructor asserts on it.
Where assertions are disabled the constructor carries on, and the entry with the empty name ends up in the config and is returned by GetFirstEntry().
Nothing is logged in either case, so an application loading a config file it did not write cannot tell that a line was bad.
src/common/fileconf.cpp:758 builds the key and never checks whether anything is left of it:
758 wxString strKey(FilterInEntryName(wxString(pStart, pEnd).Trim()));
The '=' expected branch immediately below logs and drops a malformed line, but an empty key goes straight to AddEntry:
764 if ( *pEnd++ != wxT('=') ) { 765 wxLogError(_("file '%s', line %zu: '=' expected."), 766 buffer.GetName(), n + 1); 767 } 768 else { 769 wxFileConfigEntry *pEntry = m_pCurrentGroup->FindEntry(strKey); 770 771 if ( pEntry == nullptr ) { 772 // new entry 773 pEntry = m_pCurrentGroup->AddEntry(strKey, n);
1989 wxFileConfigEntry::wxFileConfigEntry(wxFileConfigGroup *pParent, 1990 const wxString& strName, 1991 int nLine) 1992 : m_strName(strName) 1993 { 1994 wxASSERT( !strName.empty() );
Three inputs reach it, so testing the first character of the line is not enough.
The attached files, as hex:
minimal_empty_key.ini 3d 31 0a =1
minimal_empty_key_ws.ini 09 20 09 3d 31 0a tab space tab =1
minimal_empty_key_escape.ini 20 5c 20 3d 31 0a space backslash space =1
The second is skipped down to the = by the leading-space loop at line 677.
The third has a non-empty key on disk: Trim() removes the trailing space and FilterInEntryName (line 2227) drops the trailing backslash, leaving nothing.
Expected vs observed behaviour:
Expected is that the line is dropped with a diagnostic, which is what the '=' expected branch of the same if/else already does.
Observed is wxASSERT( !strName.empty() ) failing at fileconf.cpp:1994, and, once that assertion no longer stops the program, an entry with an empty name in the parsed config.
Removing the assertion on its own would therefore not fix this.
This is the same shape as the duplicate-group assertion you replaced with a warning in 3ac810a ("Warn about multiple groups in wxFileConfig instead of asserting", #26654), also found by OSS-Fuzz.
Stack trace:
#7 wxFileConfigEntry::wxFileConfigEntry(wxFileConfigGroup*, wxString const&, int) src/common/fileconf.cpp:1994
#8 wxFileConfigGroup::AddEntry(wxString const&, int) src/common/fileconf.cpp:1785
#9 wxFileConfig::Parse(wxTextBuffer const&, bool) src/common/fileconf.cpp:773
#10 wxFileConfig::wxFileConfig(wxInputStream&, wxMBConv const&) src/common/fileconf.cpp:623
Patch or snippet allowing to reproduce the problem:
The attached Poc.cpp is self-contained.
It installs an assert handler that reports and returns instead of aborting, so one run shows both the assertion and what the parser does afterwards, as a wxDEBUG_LEVEL=0 build would.
git clone https://github.com/wxWidgets/wxWidgets.git cd wxWidgets git checkout 1e8311d98f80a045f9e22fac7415c7e89356d0e7 ./configure --without-subdirs --disable-shared --disable-sys-libs --disable-gui make -j$(nproc) g++ -o Poc /path/to/Poc.cpp `./wx-config --cxxflags --libs base` ./Poc /path/to/minimal_empty_key.ini
prints
ASSERT ./src/common/fileconf.cpp:1994 "!strName.empty()"
asserts=1
entry name=[] len=0 value=[1]
--- Save() ---
=1
--- end ---
minimal_empty_key_ws.ini and minimal_empty_key_escape.ini print the same, apart from the raw line echoed back by Save().
control_wellformed.ini, which is key=1, prints asserts=0 and entry name=[key] len=3 value=[1].
To Reproduce:
The finding came from the wxwidgets oss-fuzz target fileconf, built by OSS-Fuzz with ASan against wxWidgets master at 1e8311d9.
Download Reproduce.zip
export DOCKER_DEFAULT_PLATFORM=linux/amd64 # if on mac git clone https://github.com/google/oss-fuzz.git cd oss-fuzz python3 infra/helper.py build_image wxwidgets python3 infra/helper.py build_fuzzers --sanitizer address wxwidgets python3 infra/helper.py reproduce wxwidgets fileconf crash-0002478c190f6ede36c9a16fb09f28e8b55dbef7 python3 infra/helper.py reproduce wxwidgets fileconf minimal_empty_key.ini python3 infra/helper.py reproduce wxwidgets fileconf minimal_empty_key_ws.ini python3 infra/helper.py reproduce wxwidgets fileconf minimal_empty_key_escape.ini python3 infra/helper.py reproduce wxwidgets fileconf control_wellformed.ini
The first four print SUMMARY: libFuzzer: fuzz target exited, the last one runs to completion.
The target installs an assert handler that calls exit(1) (tests/fuzz/fileconf.cpp:32), which is why an assertion shows up as an exit rather than as a crash.
crash-0002478c190f6ede36c9a16fb09f28e8b55dbef7 is the original fuzzer testcase, attached with its CASR report.
It reaches the assertion through a single line, three tabs followed by = and a 0x01 byte; that is the only line in it with an empty key, and the file contains no backslash, so no other key can be emptied by FilterInEntryName.
Suggested fix
Give the empty key its own branch beside the existing one, attached as fix.patch:
wxLogError(_("file '%s', line %zu: '=' expected."),
buffer.GetName(), n + 1);
}
+ else if ( strKey.empty() ) {
+ wxLogError(_("file '%s', line %zu: empty key name."),
+ buffer.GetName(), n + 1);
+ }
else {wxLogError matches the '=' expected message directly above it, since both mean the line is dropped.
If you prefer a warning here, wxLogWarning drops straight into the checkWarning helper of the wxFileConfig::Error test case at tests/config/fileconf.cpp:645, which is where a regression test fits.
Applied to 1e8311d9, all four crashing inputs parse with no assertion, log empty key name for the offending line, and no longer create the empty entry.
control_wellformed.ini is unchanged, and the original fuzzer testcase still yields its one valid entry with byte-identical Save() output.
Platform and version information
- wxWidgets version: 3.3.4, master at commit
1e8311d98f80a045f9e22fac7415c7e89356d0e7 - wxWidgets port: wxBase, configured with
--disable-guias the OSS-Fuzz build does - OS: OSS-Fuzz at commit
a4df12d70b5420567d893b3f53e5818a74df5db7, defaultlinux/amd64containers
Reproduce.zip holds the original fuzzer testcase and its CASR report, the three minimal files, control_wellformed.ini, Poc.cpp and fix.patch.
Found by the CISPA Fandango-Team while triaging OSS-Fuzz findings for wxwidgets.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26914)Thanks for the analysis, but it would be great if you could ask your tool to generate a unit test to add to tests/config/fileconf.cpp which will ensure that this is not only fixed now but remains fixed in the future. Could you please do it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
alex9849
alex9849 left a comment (wxWidgets/wxWidgets#26914)In the end, our tool is a fuzzer testing OSS-Fuzz targets. So it is very good at finding crashes and doesn't generate code automatically.
But I can write a test case, no problem. I would then link a PR with the fix and test over the course of this or the next week. :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.