bound datetime/time stmt param reads to the declared length

[jmestwa-coder]

get_param_length() validates only length bytes for a parameter value, but the binary-protocol datetime reader reads to[4..6] when length>4 (needs 7) and to[7..10] when length>7 (needs 11), and the time reader reads to[8..11] when length>8 (needs 12). A client sending an off-spec datetime length of 5/6/8/9/10 or a time length of 9/10/11 makes the server read past the value, out of bounds when it ends at the packet boundary. Gate the optional time and microsecond reads on the canonical encoding lengths (7/11 for datetime, 12 for time).

[gemini-code-assist]

Code Review

This pull request updates the length validation checks when parsing TIME and DATETIME parameters in sql/sql_prepare.cc to ensure proper bounds checking before reading time components and fractional seconds. There are no review comments, and I have no additional feedback to provide.

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.

[gkodinov]

Thank you for your contribution. This is a preliminary review!

This needs:

• a MDEV describing the problem, complete with steps to reproduce
• a commit message compliant with the MariaDB coding standards
• a regression test
• a rebase to the lowest affected version (this is a bug fix and not a feature).

[jmestwa-coder]

Pushed an update:

• Rebased onto 10.6, the lowest maintained branch that carries this code, and retargeted the PR base to 10.6.
• Rewrote the commit message to describe the problem and the fix.

How it triggers: get_param_length() only validates length bytes, but the datetime reader touches to[4..6] once length > 4 (needs 7) and to[7..10] once length > 7 (needs 11), and the time reader touches to[8..11] once length > 8 (needs 12). So a client sending a non-canonical temporal length (datetime 5/6/8/9/10, time 9/10/11) reads up to 3 bytes past the value, observable as an OOB read when the value sits at the end of the packet buffer.

On the regression test: the off-spec length isn't reachable through the client library, since it always encodes the canonical lengths (4/7/11 for datetime, 8/12 for time), so mysql_client_test can't drive a short length. I confirmed the bug with a small standalone harness that runs get_param_length() plus the read logic with the value placed flush against a PROT_NONE guard page: the off-spec lengths fault before the patch and exit clean after, while the valid lengths are unchanged. I can attach that harness, or wire up a crafted-packet test if there's a spot you'd prefer for one.

I'll open the MDEV with the details above and prefix the commit subject with the id.

[jmestwa-coder]

here's the standalone reproducer for the over-read, so there's a concrete repro on the thread.

it runs the datetime/time read logic from set_param_datetime() / set_param_time() with the parameter value placed flush against a PROT_NONE guard page, which stands in for a value that ends at the packet buffer boundary. get_param_length() only validates length bytes, so any read past that faults.

/* repro_param.c - binary-protocol datetime/time parameter over-read in
   Item_param::set_param_datetime() / set_param_time() (sql/sql_prepare.cc).

   build (current): cc -O0 -g repro_param.c -o repro_param
   build (patched): cc -O0 -g -DFIXED repro_param.c -o repro_param_fixed
   run:             ./repro_param <datetime|time> <length>
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <sys/mman.h>
#include <unistd.h>

#ifndef MAP_ANONYMOUS
#define MAP_ANONYMOUS MAP_ANON
#endif

typedef unsigned char uchar;

static int32_t sint4korr(const uchar *p)
{ return (int32_t)((uint32_t)p[0] | ((uint32_t)p[1] << 8) |
                    ((uint32_t)p[2] << 16) | ((uint32_t)p[3] << 24)); }
static int16_t sint2korr(const uchar *p)
{ return (int16_t)((uint16_t)p[0] | ((uint16_t)p[1] << 8)); }

static volatile unsigned long sink;

static void read_datetime(uchar *to, unsigned long length)
{
  if (length >= 4)
  {
    sink = (unsigned) sint2korr(to);   /* year  */
    sink = to[2];                      /* month */
    sink = to[3];                      /* day   */
#ifdef FIXED
    if (length >= 7)                   /* needs 7 bytes */
#else
    if (length > 4)
#endif
    {
      sink = to[4]; sink = to[5]; sink = to[6];   /* hour/min/sec */
    }
#ifdef FIXED
    sink = (length >= 11) ? (unsigned long) sint4korr(to + 7) : 0; /* needs 11 */
#else
    sink = (length > 7)  ? (unsigned long) sint4korr(to + 7) : 0;
#endif
  }
}

static void read_time(uchar *to, unsigned long length)
{
  if (length >= 8)
  {
    sink = to[0];
    sink = (unsigned) sint4korr(to + 1);
    sink = to[5]; sink = to[6]; sink = to[7];
#ifdef FIXED
    sink = (length >= 12) ? (unsigned long) sint4korr(to + 8) : 0; /* needs 12 */
#else
    sink = (length > 8)  ? (unsigned long) sint4korr(to + 8) : 0;
#endif
  }
}

int main(int argc, char **argv)
{
  if (argc != 3)
  { fprintf(stderr, "usage: %s <datetime|time> <length>\n", argv[0]); return 2; }

  const char *kind = argv[1];
  unsigned long declared = strtoul(argv[2], NULL, 10);

  long pg = sysconf(_SC_PAGESIZE);
  uchar *base = mmap(NULL, 2 * pg, PROT_READ | PROT_WRITE,
                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
  if (base == MAP_FAILED) { perror("mmap"); return 2; }
  if (mprotect(base + pg, pg, PROT_NONE)) { perror("mprotect"); return 2; }

  /* last validated byte sits right before the guard page */
  uchar *to = base + pg - declared;
  memset(to, 0, declared);

  if      (!strcmp(kind, "datetime")) read_datetime(to, declared);
  else if (!strcmp(kind, "time"))     read_time(to, declared);
  else { fprintf(stderr, "kind must be datetime or time\n"); return 2; }

  printf("ok %s length=%lu\n", kind, declared);
  return 0;
}

current code:

• datetime length 5, 6, 8, 9, 10 -> SIGBUS/SIGSEGV on the guard page (reads up to 3 bytes past the value)
• time length 9, 10, 11 -> same
• canonical lengths (datetime 4/7/11, time 8/12) -> clean

with the patch (-DFIXED) every length above exits clean and the canonical values parse identically.

on an in-tree test: libmariadb only ever encodes the canonical lengths, so mysql_client_test can't drive a short one through MYSQL_BIND, and in the real server the few bytes past the value land inside the larger net read buffer rather than on a guard page, so it's not a reliable crash outside a sanitizer. i can add a crafted COM_STMT_EXECUTE case to mysql_client_test.c that sends an off-spec length for the msan builder if you want one in the tree.

[grooverdan]

nice find. Looks like the thing that #5037 aims eventually to start finding.

[jmestwa-coder]

thanks. yeah, same class #5037 is after - the off-spec lengths only fault hard under a sanitizer or a guard page, in a normal build the few extra bytes land inside the net read buffer so it's silent. once the -fsanitize builds are running this is exactly the kind of over-read they'd surface.

still happy to land the crafted COM_STMT_EXECUTE case in mysql_client_test.c for the msan builder if you want the regression test in-tree - libmariadb won't encode a short temporal length on its own, so it has to go in as a raw packet.

[jmestwa-coder]

any update?