MDEV-40571 insufficient validation of frm data when opening a table

[vuvova]

numerous checks that the frm is valid, no OOB reads, values make sense (number of keyparts not less than number of keys, no keys means no keyparts, number of long unique fields is not larger than number of fields, fields values in the record don't overlap and don't go over record ends, and so on). most asserts were changed to if()'s.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ holyfoot
❌ vuvova
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Copilot]

Pull request overview

This PR strengthens .frm parsing/initialization in TABLE_SHARE::init_from_binary_frm_image() (and related helpers) to better reject malformed/corrupted FRM images and prevent out-of-bounds reads when opening tables.

Changes:

• Adds multiple structural validation checks while parsing keys, comments, virtual-column screen data, and field layout.
• Replaces several DBUG_ASSERT()-based assumptions with runtime validation that fails table open on invalid input.
• Tightens TYPELIB/keyname parsing and introduces new bounds tracking for typelib pointer construction.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[vuvova]

nope. it's incremented but not dereferenced before the comparison. the comparison verifies that it's valid after it was incremented

[vaintroub]

Copilot, Claude, and I, all of us would appreciate if write and increment happened after the check. Even if nothing bad happens, "check, then do something" is easier to comprehend than "Do, then check, then do some more". In addition, I personally would appreciate if complex expression *((*typelib_value_names)++)= ptr; was split and simplified into at least 2 lines. it is too much that happens on that single line, 3 dereferences, increment, and another assignment all in a single line. Would be easier to read if it was split, if only for the sake of the reader.

[vuvova]

my line is exactly the same as the old line below, so must be ok

[grooverdan]

You've removed the (uint) cast. so should be (uint) total_typelib_value_count * sizeof(char *) and sizeof(char*), not like it makes much difference, is this is the same of the array element.

[vuvova]

that's fine, it's just a quick check for null fields. vcol_screen will be properly parsed later and the error will be returned from there

[vaintroub]

Ok, but I agree with Copilot(and Claude that took a look over that code with me) about #5477 (comment) . Maybe nothing bad happens, but it is hard to tell the way the code is written. There is a modification of the return value, which might be unnecessary.
If the bounds was put before modification, nobody would stumble across it.

Otherwise, looks OK.
You'd need to fix MSVC complains about uint_variable -= size_t_variable (possible loss of data) before the push.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

sql/table.cc:2275

• multi_alloc_root() reads each length argument as uint (see mysys/my_alloc.c:324), but these two arguments are passed as size_t expressions. That’s undefined behavior for varargs and can lead to under-allocation / memory overlap on some ABIs. Also typelib_value_lengths is a uint* array, so the allocation should use sizeof(uint) (not sizeof(uint *)).

                        &interval_array, total_typelib_value_count * sizeof(char **),
                        &typelib_value_lengths, total_typelib_value_count * sizeof(uint *),

sql/table.cc:4972

• names_end-- mutates the end pointer for the entire fix_type_pointers() call. When types > 1 (e.g., ENUM/SET typelibs), this shrinks the allowed pointer range on every non-empty typelib and will reject valid input after the first typelib. Use a per-typelib limit (e.g., names_end - 1) instead of decrementing names_end.

      names_end--; // simplify the check below, reserve place for 0 at the end
      for ( ; ptr < end; )
      {
        // Now scan the next value+sep pair
        char *vend= (char*) memchr(ptr, sep, end - ptr);

sql/table.cc:117

• The forward declaration of fix_type_pointers() drops parameter names, which makes it harder to verify call sites now that the signature changed (and is inconsistent with nearby forward declarations like find_field(...)). Consider restoring parameter names for readability and to reduce the risk of mis-ordered arguments.

static bool fix_type_pointers(const char ***, const char **, uint **,
                              TYPELIB *, uint, char *, size_t);

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

sql/table.cc:2275

• multi_alloc_root() reads each length argument as uint (see mysys/my_alloc.c:324). Several arguments here are expressions whose type becomes size_t because of sizeof(...), which is undefined behavior with varargs and can truncate/misread lengths on some platforms. Cast the full product to uint for each size expression passed to multi_alloc_root().

  if (!multi_alloc_root(&share->mem_root,
                        &share->field, (uint)(share->fields+1)*sizeof(Field*),
                        &share->intervals, (uint)interval_count*sizeof(TYPELIB),
                        &share->check_constraints, (uint) share->table_check_constraints * sizeof(Virtual_column_info*),
                        &interval_array, (uint)total_typelib_value_count * sizeof(char *),
                        &typelib_value_lengths, (uint)total_typelib_value_count * sizeof(uint),

storage/connect/mysql-test/connect/t/drop-open-error.test:20

• This change removes the only remaining test coverage referencing MDEV-7935 (server crash in CREATE TABLE ... AS SELECT with CONNECT + missing file). If the underlying issue is still relevant, consider relocating/updating this test rather than dropping it entirely (e.g., adjust expectations for @@secure_file_priv and keep the CONNECT failure assertion).

--replace_result $MARIADB_DATADIR DATADIR/ './' 'DATADIR/'
drop table mdev9949;
drop table t1;

mysql-test/suite/sys_vars/t/secure_file_priv.test:36

• The new /proc/cpuinfo coverage is non-portable and will fail on Windows (no /proc, and the new default exclude list is guarded by #ifndef _WIN32). Add an explicit non-Windows guard for this part of the test.

--echo #
--echo # MDEV-40589 default exclude list for secure-file-priv
--echo #
CREATE TABLE t1 (c1 VARCHAR(50));
--error ER_OPTION_PREVENTS_STATEMENT
LOAD DATA INFILE '/proc/cpuinfo' INTO TABLE t1;
DROP TABLE t1;

sql/sys_vars.cc:3368

• The PR title/description focus on FRM validation when opening tables (MDEV-40571), but this change set also alters secure_file_priv behavior/documentation and moves unrelated stack-overrun tests. Consider splitting these into separate PRs or updating the PR description to cover the additional scope so reviewers can evaluate each change area appropriately.

static Sys_var_charptr_fscs Sys_secure_file_priv(
  "secure_file_priv",
  "Limit LOAD DATA, SELECT ... OUTFILE, and LOAD_FILE() to files "
  "within specified directory."
#ifndef _WIN32
  " Empty value means no limits except /proc"
#endif
  ,
  PREALLOCATED READ_ONLY GLOBAL_VAR(opt_secure_file_priv),
  CMD_LINE(REQUIRED_ARG), DEFAULT(0));