Static analysis fixes by FaramosCZ · Pull Request #4868 · MariaDB/server

FaramosCZ · 2026-03-26T22:21:27Z

Fixes for various static analysis discoveries of high severity or higher.

_mi_get_block_info() case 0 (BLOCK_DELETED) reads block_len from a 3-byte on-disk field (max ~16 MB) and validates minimum size and alignment, but lacks an upper-bound check. A corrupt or crafted .MYD file can supply an excessively large block_len that propagates to _mi_find_writepos(), causing an accounting underflow on info->state->empty and data corruption. Add a check against MI_MAX_BLOCK_LENGTH, consistent with the upper bound enforced in the new-block allocation path of _mi_find_writepos() and the block length limit used throughout the MyISAM dynamic record code. Co-Authored-By: Claude AI <noreply@anthropic.com>

When dlen (blob length) is less than 4 bytes, the expression dlen-4 causes an unsigned integer underflow (CWE-190) since dlen is uint. This results in a ~4 GB size being passed to sp_mbr_from_wkb(), which can cause a crash or information disclosure when processing a corrupted .MYD data file with a spatial index. Add a bounds check before subtracting the SRID size in both the MyISAM (sp_make_key) and Aria (_ma_sp_make_key) implementations, returning HA_ERR_WRONG_IN_RECORD to match the existing pattern used throughout both modules for corrupt record data. Co-Authored-By: Claude AI <noreply@anthropic.com>

Add bounds checks on field name, hostname, and table name lengths read from .cfg files via mach_read_from_4() before allocating memory with UT_NEW_ARRAY_NOKEY(). A crafted .cfg file could specify lengths up to 4 GB, causing DoS via memory exhaustion. Also add len == 0 check to the existing index name length validation. The 128-byte limit for field/column names and the FIXME comment are copied from the existing column name check in the same file (row_import_read_columns). OS_FILE_MAX_PATH is used for hostname and table name to match existing InnoDB conventions. Co-Authored-By: Claude AI <noreply@anthropic.com>

recv_spaces.find() is called at eight sites in log0recv.cc. Four of them check the result against recv_spaces.end() at runtime before dereferencing (lines 745, 765, 825, 2577). The other four rely solely on ut_ad assertions (lines 943, 1076, 3896, 4538), which are compiled out in release builds. This inconsistency means half the call sites are protected against an unexpected end-iterator and half are not. In a release build, if the invariant ever breaks at one of the unprotected sites, the end-iterator is dereferenced silently, causing undefined behavior -- potentially a crash during crash recovery. Add runtime guards after the existing ut_ad assertions at all four unprotected sites, so that every recv_spaces.find() call site now has both: - the ut_ad assertion, which fires in debug builds to flag the unexpected condition during development, and - a runtime check that safely skips or errors out in release builds. Each guard uses the existing error/skip path appropriate to its context: goto next_item, goto fail, goto nothing_recoverable, or goto next. Co-Authored-By: Claude AI <noreply@anthropic.com>

In MakeCommand(), qrystr is allocated with strlen(Qrystr)+5 bytes but later receives Query->GetStr() via strcpy at line 713. Query may be longer than qrystr when TableName is longer than Name, since Name is replaced by TableName in the query string. This causes a heap buffer overflow. Fix by: 1. Allocating qrystr with strlen(Qrystr)+64, matching the Query STRING object allocation on line 679. Add a comment linking the two. 2. Replacing the strcpy with safe_strcpy to bound the copy to the allocated size. This provides defense-in-depth: the STRING class can dynamically grow beyond its initial allocation via Realloc(), so matching the initial capacity alone is not a complete guarantee. The bounded copy ensures safety regardless of STRING's internal growth. Found by static analysis (CWE-122). Co-Authored-By: Claude AI <noreply@anthropic.com>

strcpy(result, g->Message) copies a 4160-byte g->Message buffer into the UDF result buffer which is only guaranteed to be 255 bytes by the MySQL UDF API. This can cause a heap buffer overflow when error messages exceed 255 characters. Replace all 37 instances (18 in bsonudf.cpp, 19 in jsonudf.cpp) of strcpy(result, g->Message) with safe_strcpy(result, 255, g->Message) which truncates the message to fit the destination buffer. The magic number 255 is the minimum UDF result buffer size guaranteed by the MySQL UDF API, documented in sql/udf_example.c:260 as "At least 255 byte long." The server constant MAX_FIELD_CHARLENGTH (sql_const.h) defines this same value, but sql_const.h is not included by these files and adding that include would pull in server internals. Using the literal matches the CONNECT plugin's existing style, which does not reference server constants for UDF buffer sizes. Note: a few other strcpy calls into result (e.g. strcpy(result, msg) in catch blocks, strcpy(result, ofn) for file paths) are not addressed here. Those copy from differently-sized sources and warrant separate analysis. Found by static analysis (CWE-122). Co-Authored-By: Claude AI <noreply@anthropic.com>

vaintroub · 2026-03-27T11:41:51Z

Because nobody knows how to handle Galera exceptions, std::terminate can be the right thing to do, rather than catch all/eat all exceptions. Maybe you can communicate it to Claude.

janlindstrom

Galera related exceptions should be already be handled inside a library.

The InnoDB allocator (ut0new.h) has throw(std::bad_alloc) paths. Since C++11 destructors are implicitly noexcept, an uncaught std::bad_alloc calls std::terminate(), crashing the server. Wrap code paths that can reach the InnoDB allocator in try/catch blocks catching only std::bad_alloc within three affected destructors: THD::~THD(): free_connection() -> ha_close_connection() can reach InnoDB allocator paths that throw std::bad_alloc. ~Delayed_insert(): close_thread_tables() -> mysql_unlock_tables() -> InnoDB handler chain can reach the InnoDB allocator. TR_table::~TR_table(): close_log_table() -> close_thread_tables() -> mysql_unlock_tables() -> ha_external_unlock() -> InnoDB external_lock(F_UNLCK) can trigger innobase_commit(). InnoDB allocator has throw(std::bad_alloc) paths (ut0new.h). Co-Authored-By: Claude AI <noreply@anthropic.com>

The InnoDB allocator (ut0new.h) has throw(std::bad_alloc) paths. If std::bad_alloc propagates through a noexcept function, std::terminate() is called, crashing the server. Affected functions and fixes: 1. dict_sys_t::create_or_check_sys_tables() in dict0crea.cc: calls que_eval_sql(), trx->rollback(), trx->commit() which can reach the InnoDB allocator. Extract the throwing body into create_or_check_sys_tables_impl() and wrap the call in try/catch(std::bad_alloc). 2. mdl_release() in dict0dict.cc: wrap release_lock() in try/catch(std::bad_alloc). 3. dict_stats::open() in dict0dict.cc: extract body into open_impl() and wrap in try/catch(std::bad_alloc). Cleanup partially acquired resources on exception. 4. dict_stats::close() in dict0dict.cc: wrap the release_lock() calls in try/catch(std::bad_alloc). Co-Authored-By: Claude AI <noreply@anthropic.com>

Wrap potentially-throwing InnoDB allocator paths in try/catch(std::bad_alloc) blocks within noexcept functions to prevent std::terminate() crashes on memory allocation failure: - trx_purge_close_tables(): wrap mdl_context.release_lock() - trx_purge_table_acquire(): wrap mdl_context.try_acquire_lock() Co-Authored-By: Claude AI <noreply@anthropic.com>

FaramosCZ · 2026-03-27T13:26:34Z

Right, there is no reasonable way to handle it in the server.

I dropped the galera/wsrep parts and left only unrelated exceptions handling.

janlindstrom · 2026-03-30T04:19:05Z

These changes should be reviewed by someone more familiar on code areas changed. However, if I correctly understand release builds are build with compiler variable -fno-exceptions (build_configurations/mysql_release.cmake)

In this case), try-catch blocks effectively become non-functional placeholders, and the program's behavior changes drastically when a throw statement is encountered.

dr-m

When it comes to InnoDB, I think that a separate bug and fix should be filed for improving the .cfg file parser.

As I noted in this comment, some developers may have a misconception that exceptions would be disabled when building the code. The check in include/my_global.h is misspelling __GNUC__ as __GNUC, which is why it has no effect:

#if defined(__GNUC) && defined(__EXCEPTIONS)
#error "Please add -fno-exceptions to CXXFLAGS and reconfigure/recompile"
#endif

dr-m · 2026-03-30T04:43:43Z

 void mdl_release(THD *thd, MDL_ticket *mdl) noexcept
 {
  if (thd && mdl)
-    thd->mdl_context.release_lock(mdl);
+  {
+    try
+    {
+      thd->mdl_context.release_lock(mdl);
+    }
+    catch (const std::bad_alloc&)
+    {
+      sql_print_warning(
+        "InnoDB: mdl_release():"
+        " memory allocation failure");
+    }
+  }
 }


If we fail to release a lock for whatever reason, I think that it could be best to intentionally crash the server.

@FaramosCZ release_lock() is implemented such that it doesn't return anything and it mustn't throw anything. Can we see the error text? If global -fno-exceptions doesn't work for a good reason, I'd rather make release_lock() noexcept.

Last time i checked i concluded -fno-exceptions wasn't the case since about 2012

dr-m · 2026-03-30T04:45:06Z

+		/* FIXME: What is the maximum field name length? */
+		if (len == 0 || len > 128) {
+			ib_errf(thd, IB_LOG_LEVEL_ERROR,
+				ER_IO_READ_ERROR,
+				"Field name length " ULINTPF
+				", is invalid", len);
+
+			return(DB_CORRUPTION);
+		}


The maximum ought to be NAME_LEN, or 192 bytes.

dr-m · 2026-03-30T04:50:35Z

        recv_spaces_t::iterator it {recv_spaces.find(d->first)};
        ut_ad(it != recv_spaces.end());
+        if (it == recv_spaces.end())
+          goto next_item;


I would rather not add any dead code to the recovery code path. By design, whenever we encounter a log record for a tablespace that was not previously known and cannot be found in the file system, we will add an entry to recv_spaces. This happens in log_page_modify() and fil_name_process().

dr-m · 2026-03-30T04:52:17Z

 		/* The NUL byte is included in the name length. */
 		ulint	len = mach_read_from_4(ptr);

-		if (len > OS_FILE_MAX_PATH) {
+		if (len == 0 || len > OS_FILE_MAX_PATH) {
 			ib_errf(thd, IB_LOG_LEVEL_ERROR,
 				ER_INNODB_INDEX_CORRUPT,
 				"Index name length (" ULINTPF ") is too long, "


If the comment holds, the maximum should be NAME_LEN+1, or 193 bytes.

dr-m · 2026-03-30T04:54:39Z

+	if (len == 0 || len > OS_FILE_MAX_PATH) {
+		ib_errf(thd, IB_LOG_LEVEL_ERROR,
+			ER_IO_READ_ERROR,
+			"Hostname length " ULINTPF
+			", is invalid", len);
+
+		return(DB_CORRUPTION);
+	}
+
 	/* NUL byte is part of name length. */
 	cfg->m_hostname = UT_NEW_ARRAY_NOKEY(byte, len);


According to https://stackoverflow.com/questions/32290167/what-is-the-maximum-length-of-a-dns-name the maximum length would be 253+1, counting the terminating NUL character.

We do not seem to check that there are no NUL bytes embedded in these names or that the last byte actually is a NUL byte.

ParadoxV5 · 2026-03-30T05:34:47Z

svoj

Questions inline.

svoj · 2026-03-30T11:30:54Z

+        "InnoDB: trx_purge_table_acquire():"
+        " memory allocation failure");
      goto must_wait;
+    }


All MDL allocations must be marked with std::nothrow. Which code issues std::bad_alloc?

svoj · 2026-03-30T11:32:32Z

+      sql_print_warning("~THD(): free_connection():"
+                        " memory allocation failure");
+    }
+  }


Which code does the throw?

🍿 Blind guess: @claude is the code that does the throw 💅.

svoj · 2026-03-30T11:32:40Z

+        sql_print_warning("~Delayed_insert():"
+                          " close_thread_tables():"
+                          " memory allocation failure");
+      }


Which code does the throw?

svoj · 2026-03-30T11:32:50Z

+    {
+      sql_print_warning("~TR_table():"
+                        " memory allocation failure");
+    }


Which code does the throw?

svoj · 2026-03-30T11:34:29Z

+  mdl_table= nullptr;
+  mdl_index= nullptr;
+  return true;
+}


Which code does the throw?

svoj · 2026-03-30T11:34:37Z

+    sql_print_warning(
+      "InnoDB: dict_stats::close():"
+      " memory allocation failure");
+  }


Which code does the throw?

svoj · 2026-03-30T11:34:56Z

+            "InnoDB: trx_purge_close_tables():"
+            " memory allocation failure");
+        }
+      }


Which code does the throw?

FaramosCZ · 2026-04-07T16:04:31Z

I split the .cfg file parser improvements to:
https://jira.mariadb.org/browse/MDEV-39278
#4913
as suggested.

FaramosCZ and others added 6 commits March 26, 2026 22:50

FaramosCZ marked this pull request as ready for review March 27, 2026 00:48

janlindstrom requested changes Mar 27, 2026

View reviewed changes

Comment thread sql/sql_class.h Outdated

Comment thread storage/innobase/trx/trx0roll.cc Outdated

FaramosCZ and others added 3 commits March 27, 2026 14:07

FaramosCZ force-pushed the 2026_static_analysis_fixes branch from 84f73d7 to b4a0cc2 Compare March 27, 2026 13:22

gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Mar 27, 2026

FaramosCZ requested a review from janlindstrom March 28, 2026 12:50

janlindstrom requested review from vuvova and removed request for janlindstrom March 30, 2026 04:19

janlindstrom assigned vuvova Mar 30, 2026

dr-m reviewed Mar 30, 2026

View reviewed changes

ParadoxV5 reviewed Mar 30, 2026

View reviewed changes

Comment thread sql/sql_class.cc

Copy link
Copy Markdown

Contributor

ParadoxV5 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍿

FaramosCZ marked this pull request as draft March 30, 2026 10:06

svoj reviewed Mar 30, 2026

View reviewed changes

Uh oh!

Conversation

FaramosCZ commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaintroub commented Mar 27, 2026

Uh oh!

janlindstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FaramosCZ commented Mar 27, 2026

Uh oh!

janlindstrom commented Mar 30, 2026

Uh oh!

dr-m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ParadoxV5 Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FaramosCZ commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

9 participants

FaramosCZ commented Mar 26, 2026 •

edited

Loading

ParadoxV5 Mar 30, 2026 •

edited

Loading