bb-10.6-rucha#5429
Closed
mariadb-RuchaDeodhar wants to merge 37 commits into
Closed
Conversation
Correct append function called for value in quotes
The database name in GRANT is stored in mysql.db.Db, which holds 64 characters. Escaping '_' or '%' with a backslash to match them literally can make the stored name longer than 64 characters, and it was silently truncated into a non-functional grant. Reject it with ER_WRONG_DB_NAME instead, the same error CREATE DATABASE gives for an over-long name.
The dynamic column integer decoders derive the shift exponent from the data interval length without bounding it to the 64-bit type width. dynamic_column_uint_read() loops over the interval doing value+= data[i] << (i*8). A record whose integer column has an interval longer than 8 bytes drives i*8 to 64 and past it, which is undefined and aborts under -fsanitize=shift. dynamic_column_var_uint_get(), used for the charset id of a string value and for the intg/frac of a decimal, has the same defect: length*7 grows without bound over a run of 0x80 continuation bytes. Reject integers longer than 8 bytes with ER_DYNCOL_FORMAT, propagate that through dynamic_column_sint_read(), and cap the varint loop at 10 groups. To reproduce, build with -fsanitize=shift and read with COLUMN_GET() a record whose integer column data interval exceeds 8 bytes; the shift exponent reaches 64 in dynamic_column_uint_read(). A new case in the ma_dyncol unit test crafts such a record and checks it is rejected.
The .gitmodules file still uses the old Codership Github repo link. This redirects to the mariadb-corporation repo, so it isn't a functional problem, but it should still be updated to the correct repo. Reviewed-by: Teemu Ollaka <teemu.ollakka@mariadb.com>
Problem: Infer found file descriptors left open on some error code paths in connect storage engine. Fix: Close file descriptors on error before returning.
Add bounds checks to prevent out-of-bounds memory reads when parsing Table_map_log_event optional metadata and constructor fields. The checks prevent OOB reads but do not attempt to throw errors for every possible corruption. Replication fundamentally assumes a trusted source with valid data. The goal here is hardening against accidental corruption (truncated events, garbled metadata lengths). On detecting corruption, the slave either falls back gracefully to positional column mapping (parse_* functions) or rejects the event entirely (constructor), rather than crashing or leaking heap contents via OOB reads. parse_* functions: Add if (unlikely(p > end)) return after every net_field_length() call. Ensures decoded values are validated before being stored to struct fields, preventing information disclosure from OOB reads. On early return, the slave gracefully falls back to positional column mapping. Table_map_log_event constructor: Add bounds checks before memcpy calls for m_colcnt, m_field_metadata_size, and m_null_bits. On overflow, frees allocated memory and sets m_memory=NULL causing is_valid() to return false. Adds read-side DBUG_EXECUTE_IF injection points for testing. Functions fixed: - parse_default_charset - parse_column_charset - parse_column_name - parse_set_str_value - parse_geometry_type - parse_simple_pk - parse_pk_with_prefix - Table_map_log_event constructor (m_colcnt, m_field_metadata, m_null_bits) MTR test rpl_table_map_log_event_overflow covers all affected code paths with 9 test cases using debug injection.
Stack control added.
Problem: The current implementation of the sysvinit script only creates `log_success_msg()` and `log_failure_msg()` fallback functions when not finding an /etc/init.d/functions file to include. Newer RHEL releases do not define those functions in that file anymore. Fix: Check if the functions are defined and provide fallback functions if they are not defined.
Issue: The SST TLS options are not role-specific, so a node cannot
present a different certificate as donor (TLS client) than as joiner
(TLS server), which some environments require.
Solution: In the shared check_server_ssl_config, add role-specific
options that override the generic ones: ssl-server-{ca,capath,cert,key}
for the joiner and ssl-client-{ca,capath,cert,key} for the donor.
Applies to both mariabackup and rsync SST.
…mediate CA Issue: Verify_ca_matches_cert ran "openssl verify -CAfile <ca> <cert>", which treats only the first cert in the file as the target. For a leaf+intermediate bundle against a root-only CA, openssl cannot reach the root, so the check aborts the SST (exit 22). Solution: Pass the cert file as -untrusted so bundled intermediates complete the chain to the CA. Trust anchor is unchanged (still -CAfile only).
…ion differently Issue: The mariabackup SST script enabled a peer certificate name check for any ssl-mode starting with VERIFY, so VERIFY_CA behaved like VERIFY_IDENTITY, unlike the rsync script. Solution: For VERIFY_CA verify the chain only - drop the socat "commonname" check in mariabackup and gate the rsync checkHost/checkIP on VERIFY_IDENTITY.
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces various bug fixes and robustness improvements, including stack overrun checks in XML functions, boundary validation in replication table map log events, directory descriptor leak fixes in the Connect storage engine, and correct SSL/TLS verification in Galera SST scripts. It also prevents silent truncation of long database names in grants and fixes single-quote escaping in directory paths. Feedback on the changes suggests optimizing my_xpath_parse_OrExpr in sql/item_xmlfunc.cc by utilizing the existing xpath->thd pointer instead of calling the thread-local current_thd lookup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| Check for stack ovver run because it might call itself and | ||
| result in stack overflow. Return error if thats the case. | ||
| */ | ||
| if (check_stack_overrun(current_thd, STACK_MIN_SIZE , NULL)) |
Contributor
There was a problem hiding this comment.
Avoid calling thread-local lookups like current_thd in performance-critical paths (such as parsing/frequent lookups). Since xpath->thd is already available in the MY_XPATH structure, use it directly to reduce overhead.
if (check_stack_overrun(xpath->thd, STACK_MIN_SIZE, NULL))References
- Avoid calling thread-local lookups like
current_thdorthd_to_trxin performance-critical paths (such as per-record operations or frequent lookups). Instead, pass down or cache the transaction pointer (trx_t) to reduce overhead.
…ion differently Add missing debug rdiff for galera_sst_mariabackup_verify_ca.
mysqltest substituted variables in the file name of write_file and append_file, but not in the content, so tests had to fall back to "exec echo ... >> file" to put a computed value into a file. Add an optional third parameter, eval, which makes the content go through do_eval(). It is off by default, because existing tests write content that contains a literal $ or backslash.
The test was trying to update the performance_schema.threads row for the current connection. And if that row was not found or was the same, there's nothing to update hence no update not allowed error. MariaDB has a device to skip calling ha_update_row() if the old and the new records are identical. The connection id is increasing by at least 2 for every test run. Eventually it will hit the constant 12 in the UPDATE and the server will not call ha_update_row() at all and won't get the error. This is not a bug: it's reasonable behavior. Stablizied the test by changing the UPDATE value to thread_id + const. Co-authored-by: Grok
…_key_server make sure pkill doesn't kill itself. it's documented not to do that, but if it's not called directly (e.g. my_safe_process -> sh -> pkill) it can still kill its parents.
check that the keyseg lengths are sane. fix Aria too.
don't truncate to a position somewhere between [old_length, max_length-1] if old_length == max_length. Simply set the length to old_length in this case. Also, max_length cannot be zero.
…imits on server results replace strmov/strxmov with strnmov, strmake, or my_snprintf
InnoDB used to have two versions of innobase_quote_identifier(). One that handled embedded backticks and one that didn't. Let's use just one implementation that handles backticks. Keep the other one as a helper that writes to FILE but does not implement the quoting logic.
Three problems: 1. Option value specified as `foo bar` was not quoted, printed as foo bar, breaking CREATE TABLE. 2. Option value that included */ was printed as is, in unknown options when the whole option was commented out, this broke the comment: /* opt='val*/ue' */ 3. Option names can include */ too, `opt*/ion`=1 Fix: simplify value printing, ignore the original quoting, always print the value quoted as a string. Break comment markers with an escape (as *\/) if inside a comment. As \-escapes don't work inside, backticks, we cannot use `-quoting for option values at all. For the same reason we cannot allow */ inside option names, thus let's only allow keywords for option names and reject fancy names even if IGNORE_BAD_TABLE_OPTIONS is in force.
use append_unescaped() for '-quoted strings in SHOW CREATE USER and SHOW GRANTS. Use append_identifier() for user- and hostnames.
covers: * DROP TRIGGER * CHANGE MASTER * CREATE FUNCTION * CREATE SERVER * INSTALL PLUGIN
before `data+= WKB_HEADER_SIZE` verify that there's enough data. rewrite this line in Gis_multi_line_string::get_mbr to make clear that it does not need this check.
…int32) (1) <= Alloced_length' failed in AsText Geometry::as_wkt() didn't take into account bytes written into the result string in the recusrive call of get_data_as_wkt(). Gis_geometry_collection::get_data_as_wkt() didn't reserve space at all. Thus nested geometry collections could overflow the buffer.
encode table name in file name in DATA/INDEX DIRECTORY
tell curl to not try to interpret url as a parameter
… on disk 1. Only allow #mysql50# prefix on table names that can not be decoded from a file name to a table name. It is an error to put this prefix on a name that can be accessed as a valid table name without a prefix. Use `t1` not `#mysql50#t1`, use `a-b` not `#mysql50#a@002db`. 2. Don't allow creation of new tables with the #mysql50# prefix. Except when in SQLCOM_ALTER_DB_UPGRADE - main.upgrade test runs ALTER DATABASE `#mysql50#mysqltest-1` UPGRADE DATA DIRECTORY NAME; This command moves tables from the old db name to the new db name, and some of these tables also have old #mysql50# names. 3. Also don't allow to create such databases, views, partitions, subpartitions, sequences, triggers.
…f too long * move all common fn_format flags that mi_create/maria_create use into one single #define. * simplify (MY_APPEND_EXT is redundant and MY_REPLACE_EXT does nothing when no . is present in the filename) * use MY_SAFE_PATH everywhere (return NULL if too long) * set my_errno= ENAMETOOLONG on too long * abort mi_create/maria_create if returned NULL
…xed-len buffer use size-bounded scanf formats
…'s privileges empty password and empty plugin name don't mean "authentication was not specified", they mean "empty authentication was specified"
Materialized_cursor included MEM_ROOT where it itself (Materialized_cursor) was allocated. Materialized_cursor inherits from Server_side_cursor, and Server_side_cursor::operator delete frees this mem_root. This offends MSAN with poison_in_dtor, because when Materialized_cursor is destroyed, MEM_ROOT is poisoned and Server_side_cursor::operator delete accesses memory it really shouldn't. Fix: * MEM_ROOT is moved into Server_side_cursor. Materialized_cursor is the only class inheriting from it, so nobody is affected. * operator delete is replaced with a function cdestroy() which saves the MEM_ROOT, deletes the object, then frees the root. * cannot close() from ~Materialized_cursor anymore, main_mem_root should be copied before the destruction
escape user and host names in view frm in fact, there is no reason to write any field as is, unescaped, so let's remove FILE_OPTIONS_STRING in the next release
Analysis: The stack size if insufficient and there is no stack overrun check. Fix: Add stack overrun check.
mariadb-RuchaDeodhar
force-pushed
the
bb-10.6-rucha
branch
from
July 22, 2026 14:26
7b2ed81 to
04ba5ac
Compare
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
12 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.
No description provided.