AVRO-4312: [c++] Validate field names and enum symbols when parsing#3889
Open
iemejia wants to merge 6 commits into
Open
AVRO-4312: [c++] Validate field names and enum symbols when parsing#3889iemejia wants to merge 6 commits into
iemejia wants to merge 6 commits into
Conversation
Record field names and enum symbols were not validated against the Avro name grammar when compiling a schema, unlike named-type simple names which are already checked by Name::check(). As a result an out-of-spec string could be emitted verbatim as a C++ identifier by the code generator. Validate field names and enum symbols during schema compilation, rejecting anything that is not a non-empty sequence of [A-Za-z0-9_]. Add negative parser tests covering spaces and identifier-injection attempts in field names and enum symbols.
There was a problem hiding this comment.
Pull request overview
This PR addresses AVRO-4312 by adding schema-compile-time validation for record field names and enum symbols in the C++ implementation, preventing out-of-spec strings from reaching avrogencpp where they could be emitted verbatim as C++ identifiers.
Changes:
- Add
validateSimpleName()to validate record field names and enum symbols during schema compilation. - Enforce validation in
makeField()(record fields) andmakeEnumNode()(enum symbols). - Extend
SchemaTestswith negative parser cases covering invalid names (spaces and identifier/code injection attempts).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lang/c++/impl/Compiler.cc | Adds and applies name validation for record field names and enum symbols during schema compilation. |
| lang/c++/test/SchemaTests.cc | Adds negative schema parser tests for invalid field names and enum symbols. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+144
to
+149
| for (const char c : name) { | ||
| if (!std::isalnum(static_cast<unsigned char>(c)) && c != '_') { | ||
| throw Exception("Invalid {} name: {}", what, name); | ||
| } | ||
| } | ||
| } |
Address review feedback: validateSimpleName used std::isalnum, whose result depends on the current locale and could accept characters outside the intended ASCII [A-Za-z0-9_] grammar on non-"C" locales. Check the ASCII letter/digit ranges explicitly instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
lang/c++/impl/Compiler.cc:140
- The comment says the same rule is "already enforced" by Name::check(), but Name::check() currently uses std::isalnum (locale-dependent and not restricted to ASCII). Since validateSimpleName() intentionally restricts checks to ASCII, it would be clearer to avoid implying identical behavior.
// Validates that a record field name or enum symbol conforms to the Avro name
// grammar (a non-empty sequence of [A-Za-z0-9_], as already enforced for named
// type simple names by Name::check()). This prevents out-of-spec strings from
// being emitted verbatim as identifiers by the C++ code generator. The character
// checks are restricted to ASCII on purpose (rather than std::isalnum, which is
// locale-dependent) so the accepted grammar does not vary with the locale.
Comment on lines
+235
to
+243
| // Names outside the Avro name grammar | ||
| // Enum symbol containing a space | ||
| R"({"type": "enum", "name": "Status", "symbols" : ["Ok", "Not Ok"]})", | ||
| // Enum symbol attempting identifier injection | ||
| R"({"type": "enum", "name": "Status", "symbols" : ["Ok", "A, B_c = 5"]})", | ||
| // Field name containing a space | ||
| R"({"type":"record","name":"R","fields":[{"name":"in valid","type":"long"}]})", | ||
| // Field name attempting identifier/code injection | ||
| R"({"type":"record","name":"R","fields":[{"name":"x; int y","type":"long"}]})" |
Address review feedback: cover the non-empty constraint enforced by validateSimpleName() with explicit empty field name and empty enum symbol schemas.
Comment on lines
+136
to
+140
| // grammar (a non-empty sequence of [A-Za-z0-9_], as already enforced for named | ||
| // type simple names by Name::check()). This prevents out-of-spec strings from | ||
| // being emitted verbatim as identifiers by the C++ code generator. The character | ||
| // checks are restricted to ASCII on purpose (rather than std::isalnum, which is | ||
| // locale-dependent) so the accepted grammar does not vary with the locale. |
Address review feedback: Name::check() (used for named-type simple names and namespaces) relied on std::isalnum, which is locale-dependent, while the new field/symbol validation uses an ASCII check. Introduce a shared-style locale-independent ASCII predicate for the name-character tests in Name::check() so all schema name elements are validated with the same locale-invariant [A-Za-z0-9_] grammar.
Comment on lines
449
to
450
| validateSimpleName(it.stringValue(), "enum symbol"); | ||
| symbols.add(it.stringValue()); |
Address review feedback: store the enum symbol string once instead of calling Entity::stringValue() (which performs a type check and conversion) both for validation and when adding the symbol.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
lang/c++/impl/Node.cc:125
- Name::check() (and by extension schema compilation for named types) accepts simple names that start with a digit. Since avrogencpp emits named types as C++ identifiers (decorate() only adjusts reserved keywords), this can still lead to generated C++ that won't compile. Consider rejecting simple names whose first character is not [A-Za-z_].
void Name::check() const {
if (!ns_.empty() && (ns_[0] == '.' || ns_[ns_.size() - 1] == '.' || std::find_if(ns_.begin(), ns_.end(), invalidChar1) != ns_.end())) {
throw Exception("Invalid namespace: " + ns_);
}
if (simpleName_.empty()
|| std::find_if(simpleName_.begin(), simpleName_.end(), invalidChar2) != simpleName_.end()) {
throw Exception("Invalid name: " + simpleName_);
}
Comment on lines
+135
to
+151
| // Validates that a record field name or enum symbol conforms to the Avro name | ||
| // grammar (a non-empty sequence of [A-Za-z0-9_], as also enforced for named type | ||
| // simple names by Name::check()). This prevents out-of-spec strings from being | ||
| // emitted verbatim as identifiers by the C++ code generator. The character checks | ||
| // are restricted to ASCII (rather than the locale-dependent std::isalnum), which | ||
| // is consistent with the locale-independent checks used by Name::check(). | ||
| static void validateSimpleName(const string &name, const char *what) { | ||
| if (name.empty()) { | ||
| throw Exception("Empty {} name", what); | ||
| } | ||
| for (const char c : name) { | ||
| const bool isAsciiAlnum = (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z'); | ||
| if (!isAsciiAlnum && c != '_') { | ||
| throw Exception("Invalid {} name: {}", what, name); | ||
| } | ||
| } | ||
| } |
Address review feedback: names starting with a digit (e.g. "1abc") were accepted but would be emitted as invalid C++ identifiers by avrogencpp. Enforce the Avro rule that a simple name's first character must be [A-Za-z_] in both validateSimpleName (field names and enum symbols) and Name::check (named-type simple names), keeping the two consistent. Add negative tests for leading-digit field, enum symbol and type names.
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
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.
What is the purpose of the change
Record field names and enum symbols were not validated against the Avro name
grammar when compiling a schema, unlike named-type simple names which are
already checked by
Name::check(). As a result an out-of-spec string could beemitted verbatim as a C++ identifier by the code generator (
avrogencpp).This change validates record field names and enum symbols during schema
compilation, rejecting anything that is not a non-empty sequence of
[A-Za-z0-9_](the same rule already applied to named-type simple names).Part of AVRO-4312.
Verifying this change
This change added tests and can be verified as follows:
test/SchemaTests.cc(basicSchemaErrors)covering spaces and identifier-injection attempts in both field names and
enum symbols.
SchemaTests(185 cases,including the new negatives),
CompilerTests,unittest, andCommonsSchemasTests(which parses the shared interop schemas) all pass.Documentation