MDEV-39278 Validate .cfg file parser string lengths in InnoDB import#4913
MDEV-39278 Validate .cfg file parser string lengths in InnoDB import#4913gkodinov merged 1 commit intoMariaDB:10.11from
Conversation
FaramosCZ
force-pushed
the
MDEV-39278
branch
5 times, most recently
from
9091fb0 to
ebef28b
Compare
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
I see that this is pretty horrible code, which you fixed in a minimal way. Given that the change is so small, I think that should count as a bug fix and target the earliest major version branch where such fixes are accepted. That would be 10.11, because by our current policy we only fix demonstrated crashing bugs in the 10.6 release series, which will soon reach its end of life.
I hope that some day we’ll have a chance to implement MDEV-11658 and make the .cfg files redundant by storing all the metadata that is omitted from .frm files in the .ibd files themselves. This is why I don’t think it would make sense to invest time in cleaning this up further. Cleanup ideas would include making row_import_cfg_read_string() issue a single fread, and having row_import_cfg_read_index_fields(() read into an implicitly aligned uint32_t row[3] and decoding the data with my_betoh32(row[0]) and so on.
Can you please rebase this on the 10.11 branch?
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Is it possible to do even some minimal testing? E.g. have a cooked .cfg file with a large value and see it rejected?
Otherwise, please keep working with Marko on the final review. I side with his request to rebase this to 10.11.
|
|
Replace overly permissive or missing length limits in the .cfg metadata file parser (used by ALTER TABLE ... IMPORT TABLESPACE) with correct constants: - Field/index/column names: NAME_LEN + 1 (193 bytes including NUL), matching the maximum identifier length defined in mysql_com.h. Replaces the hardcoded 128 for columns (with FIXME) and OS_FILE_MAX_PATH (4000) for index names. Adds missing validation for field names. - Hostname: HOSTNAME_LENGTH + 1 (256), consistent with MariaDB's own hostname limit defined in mysql_com.h. RFC 1035 defines the textual DNS name limit as 253 characters (254 with NUL), but HOSTNAME_LENGTH (255) is based on the RFC 1034 wire-format limit of 255 octets. Using HOSTNAME_LENGTH avoids rejecting .cfg files exported from servers with valid 254-255 character hostnames. - Table name: MAX_FULL_NAME_LEN + 1 (655 bytes including NUL), since the .cfg file stores the full db/table name (written by row0quiesce.cc as table->name.m_name). Without these checks, a crafted .cfg file could specify lengths up to 2^32 via the 4-byte mach_read_from_4() length prefix, causing excessive memory allocation. Use ib_senderrf() instead of ib_errf() for reporting validation failures. ib_errf() pre-formats its message into a single string and passes it to ib_senderrf(), but ER_IO_READ_ERROR expects three arguments (%lu, %s, %s). Using ib_senderrf() directly with the correct arguments avoids this format mismatch. The pre-existing column name check had the same ib_errf() misuse and is corrected here as well. The test covers hostname and table name length validation. Column name, index name, and field name length validation are not tested because their offsets are deep in the .cfg binary format and would require walking past variable-length sections (autoinc, page size, flags, column/index metadata). Co-Authored-By: Claude AI <noreply@anthropic.com>
|
I rebased to 10.11, switched to the better |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Thanks for working on the test!
4 participants
Replace overly permissive or missing length limits in the .cfg metadata file parser (used by ALTER TABLE ... IMPORT TABLESPACE) with correct constants:
Field/index/column names: NAME_LEN + 1 (193 bytes including NUL), matching the maximum identifier length defined in mysql_com.h. Replaces the hardcoded 128 for columns (with FIXME) and OS_FILE_MAX_PATH (4000) for index names. Adds missing validation for field names.
Hostname: HOSTNAME_LENGTH + 1 (256), consistent with MariaDB's own hostname limit defined in mysql_com.h. RFC 1035 defines the textual DNS name limit as 253 characters (254 with NUL), but HOSTNAME_LENGTH (255) is based on the RFC 1034 wire-format limit of 255 octets. Using HOSTNAME_LENGTH avoids rejecting .cfg files exported from servers with valid 254-255 character hostnames.
Table name: MAX_FULL_NAME_LEN + 1 (655 bytes including NUL), since the .cfg file stores the full db/table name (written by row0quiesce.cc as table->name.m_name).
Without these checks, a crafted .cfg file could specify lengths up to 2^32 via the 4-byte mach_read_from_4() length prefix, causing excessive memory allocation.