Repository navigation
fix: guard front()/back() access on empty strings and containers - #1190
Merged
Merged
Conversation
The old implementation called front()/back() unconditionally, so trimming an empty string was undefined behaviour. It also erased one character at a time from the front, which is O(n^2) for long strings. Trim the tail in a loop guarded by an emptiness check, and drop the leading whitespace in a single erase using find_if_not.
The length guard tested the input `x` instead of the split-off quantum number `out.first`, which can be shorter than `x` after stripping. That left two undefined-behaviour paths: - Empty `out.first`: calling `front()` on an empty string_view - Single-character `out.first`: reading `operator[1]` past the end The guard now tests `out.first.size() > 1`, so both accesses are in range. Also normalise the comparisons to left-hand-value order.
basename.back() was called twice without checking for emptiness, so an empty `basename` workspace argument invoked undefined behaviour. An empty basename is already a complete prefix: it means the working directory and the file name is used as is. That is what the two other prepend sites already do, `m_xsec_fit.cc` and `m_predefined_absorption_models.cc` both guard with `basename.length() &&` and leave the prefix empty. The name stays relative, so the include and data path search still applies. The fix sits in the shared helper rather than at each caller, so all three call sites are covered: the two LBL methods in `m_lbl.cc` and the atmospheric method in `m_atm.cc`.
basename.back() was used on the user-supplied workspace
string without checking it first, which is undefined
behaviour when the basename is empty.
This path does not go through complete_basename, it is
built with std::filesystem::path. There an empty basename
is not the same as the current directory: is_directory("")
is false, so the separator branch appends "." and the
lookup lands on a dot-prefixed name such as
.N2-CIA-N2.xml. Treating an empty basename as the current
directory instead matches complete_basename and the
m_xsec_fit.cc prepend site, where an empty basename means
no prefix and the file name is used as is.
The empty-description check only records an error and does not short-circuit, so the following check called .back() on an empty string, which is undefined behaviour. Guard the newline check with a non-empty test so the access only happens when a description exists.
`split()` skips empty tokens, so an empty or comma-only gin_type/gout_type entry returns an empty ArrayOfString, and calling `.front()` on it is undefined behaviour. The validation loop above cannot catch this: it iterates over the split result, which is empty, so no item is ever checked and the empty type slips through. Hoist the split result into a local, push an error when it is empty, and only take `.front()` afterwards.
Calling desc.back() on an empty description is undefined behaviour. Empty descriptions are legal at this stage; they are flagged separately by the metadata error scan, so just skip the newline append when the description is empty. This mirrors the guard already used by fix_static in src/core/options/arts_options.cc.
The empty() check sat after a validation lambda that already calls values_and_desc.front(), so an option with no values hit undefined behaviour instead of the intended error. Move the check above the lambda so the throw happens first.
EnumeratedOption::docs() and tail() called values_and_desc.front() without checking emptiness, and docs() also called v.back() on each inner vector unchecked, which is undefined behaviour for empty enum classes. Throw instead, reusing the "No values for enum class" and "Empty value for enum class" diagnostics already raised by head().
The binding generator dereferenced empty containers while emitting enum classes: - values_and_desc.front() is used twice to size the get_options_as_strings() switch, with no outer-emptiness check on the option. - value.front()/value.back() feed the property name and its docs, and value.size() - 1 underflows for an empty inner vector. - nonstd::isdigit(x.front()) read a possibly empty option string; short-circuit that case into the existing skip. The two vector guards throw with the same wording used by arts_options.cc, so bad enum metadata now fails the build with a named error instead of undefined behaviour.
Calling nonstd::isdigit(info.code.front()) on an empty code is undefined behaviour. Check for emptiness first and throw a std::runtime_error naming the record, matching the error style already used in this file.
Calling front() on an empty std::string is undefined behaviour, and the argument names here can legitimately be empty. Five prefix tests compared front() against the internal/named-input prefixes; they now use std::string::starts_with, which is safe on empty strings and already the idiom used in workspace_method_class.h. At the two sites that also slice the string, the short-circuit previously evaluated begin() + 1 on an empty string, a second latent UB that starts_with removes as well. - Method ctor: is_gname() common G-name test - Method ctor: required GINS check - add_defaults_to_agenda(): default insertion loop - sphinx_list_item(): output and input setval loops
Both xml_parse_error and xml_data_parse_error throw unconditionally, but nothing in their signatures said so. Adding the attribute makes the existing name.empty() guard in XMLTag::check_end_name verifiable: without it, the name.front() call right after the guard reads as unguarded undefined behaviour to readers and static analysers. Placement follows the existing [[noreturn]] usage in src/m_lbl.cc. Four translation units were syntax-checked with no new warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch removes a class of undefined behavior spread across the core, workspace, and code-generator sources: calling
front(),back(), oroperator[]on strings and containers that may legitimately be empty. Each site is now either explicitly guarded, validated with a clear error message, or rewritten to use the saferstarts_with/ends_withforms. The fixes make the workspace-method and option generators, the species table generator, and the XML/CIA file-name handling robust against empty input instead of relying on unspecified behavior. A small chore marks the XML parse error handlers[[noreturn]]so that control flow after them is properly understood by the compiler and static analyzers.Note: This is only the first part of the cleanup — additional fixes for remaining UB sites will follow in a later PR.
Changes
src/core/strings/mystring.cpp:trim()is now safe for empty strings — trailing whitespace is removed only while the string is non-empty, and leading whitespace is stripped with a singleerasebased onfind_if_not.src/core/quantum/quantum.cc:split_hitran_qn()tests the length of the extracted first quantum-number label itself rather than the input string, sofront()and[1]are only read when the label actually has those characters.src/core/spec/make_species.cc: the isotopologue table writer now throws a descriptive error for an empty isotopologue code instead of readingcode.front()unconditionally.src/core/options/arts_options.cc:EnumeratedOption::docs(),tail(), andhead()validate thatvalues_and_descis non-empty (and that each value entry is non-empty) before usingfront()/back(); the existing empty check inhead()was moved ahead of its first use.src/python_interface/gen_auto_py_options.cpp:enum_option()rejects enum classes without values and skips empty value strings before inspectingx.front(), so generated Python bindings stay well defined for degenerate enum declarations.src/make_auto_wsm.cpp:scan_wsmr_for_errors()only checks the trailing newline when a description is non-empty, andscan_for_errors()reports an "Empty group type" error instead of takingfront()of a possibly emptysplit()result.src/workspace_methods.cpp:fix()appends the trailing newline to a method description only when the description is non-empty.src/workspace_method_class.cpp: prefix tests on argument names usestarts_with()for the internal and named-input prefixes instead of comparingfront(), removing UB on empty argument strings.src/core/xml/xml_io.cpp:complete_basename()returns an empty basename unchanged, treating it as "no prefix" rather than inspectingback().src/m_cia.cc: the split-catalog CIA reader treats an empty basename as a directory and usesends_with('/')for the trailing-slash test.src/core/xml/xml_io_base.handsrc/core/xml/xml_io_base.cpp:xml_parse_error()andxml_data_parse_error()are annotated[[noreturn]], matching the fact that both always throw.Breaking Changes
None. All changes only affect inputs that previously triggered undefined behavior; valid inputs behave exactly as before.