Skip to content

MDEV-40996 Support --sync_with_master 0, $variable in mysqltest - #5619

Open
ParadoxV5 wants to merge 1 commit into
10.11from
px5.sync_with_master-var
Open

MDEV-40996 Support --sync_with_master 0, $variable in mysqltest#5619
ParadoxV5 wants to merge 1 commit into
10.11from
px5.sync_with_master-var

Conversation

@ParadoxV5

@ParadoxV5 ParadoxV5 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

--sync_with_master uses get_string(), which has $variable support, but it only uses the read buffer, which is written with the unexpanded string and not the variable value.

@vuvova

vuvova commented Sep 1, 2026

Copy link
Copy Markdown
Member

@ParadoxV5 mtr/mariadb-test changes can (and should) go into the earliest version, 10.11. They don't affect the server or production use, and it's rather annoying to remember what mtr feature was added in what version and adjust tests when merging up. Better keep mtr/mariadb-test the same everywhere whenever possible

@vuvova
vuvova requested a review from KhaledR57 September 1, 2026 08:05

@KhaledR57 KhaledR57 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you also add a test? nothing exercises this path today, which is why CI is green. Two more things: please put the MDEV number first on the PR title and commit, and target 10.11 per @vuvova's comment, thanks

Comment thread client/mysqltest.cc Outdated
buff= (char*)my_malloc(PSI_NOT_INSTRUMENTED, strlen(p)+1,
MYF(MY_WME|MY_FAE));
get_string(&buff, &p, command);
start= get_string(&buff, &p, command);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

start does double duty here, it is both the value passed to do_sync_with_master2() and the pointer freed at 5405. when the argument is a $variable, get_string() returns var->str_val, so my_free(start) frees the variable's own string. also the malloc buffer leaks too, since get_string() moved buff to to+1.

--sync_with_master 0,$conn_name aborts under ASAN at exit

==117736==ERROR: AddressSanitizer: heap-use-after-free
    #0 my_free            mysys/my_malloc.c:204
    #1 var_free           client/mysqltest.cc:2796
    #4 free_used_memory() client/mysqltest.cc:1878
    #5 cleanup_and_exit   client/mysqltest.cc:1926

so you can keep start assigned the allocation base alongside buff, to free at the end, and add a separate variable that takes the get_string() return and is passed to do_sync_with_master2(). The free then always targets this function's own buffer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🫠 thank you

This is so hard to unsee now that I see it:

  if (buff)
    my_free(start);

@ParadoxV5 ParadoxV5 Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, @KhaledR57.

I’ve had fun here and will be on vacation starting the day after tomorrow, so feel free to take the branch over, GSoC colleague 😀.

@ParadoxV5
ParadoxV5 changed the base branch from 11.4 to 10.11 September 2, 2026 03:26
`--sync_with_master` uses `get_string()`,
which has `$variable` support, but it only uses the read buffer,
which is written with the unexpanded string and not the variable value.

Reviewed-by: KhaledR57 <khaled57.dev@gmail.com>
@ParadoxV5
ParadoxV5 force-pushed the px5.sync_with_master-var branch from 153ee96 to 8b8d062 Compare September 2, 2026 04:42
@ParadoxV5 ParadoxV5 changed the title MTR: Support --sync_with_master 0, $variable MDEV-40996 Support --sync_with_master 0, $variable in mysqltest Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants