MDEV-38010: Fix trailing garbage in master.info numeric fields#4764
MDEV-38010: Fix trailing garbage in master.info numeric fields#4764ayush-jha123 wants to merge 1 commit intoMariaDB:10.11from
Conversation
ayush-jha123
force-pushed
the
fix-branch-10.11
branch
from
40bdc9f to
3ba276c
Compare
gkodinov
added
the
External Contribution
|
See also #4752 |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
You could have done the target branch replace in-PR. But OK, I've closed the other one. Hope that's OK.
Please fix the msan failure. And also use the mysys functions please.
| while (my_isspace(&my_charset_bin, *endptr)) | ||
| endptr++; | ||
| if (*endptr != '\0') | ||
| DBUG_RETURN(1); |
There was a problem hiding this comment.
I believe you need to assign something to *var here. Otherwise msan complains.
sql/slave.cc
Outdated
| { | ||
| *var = atoi(buf); | ||
| char *endptr; | ||
| long val= strtol(buf, &endptr, 10); |
sql/slave.cc
Outdated
| { | ||
| if (sscanf(buf, "%f", var) != 1) | ||
| int bytes_read= 0; | ||
| if (sscanf(buf, "%f%n", var, &bytes_read) != 1) |
ayush-jha123
force-pushed
the
fix-branch-10.11
branch
5 times, most recently
from
eb5cd72 to
9d39733
Compare
|
@gkodinov Thank you for your patience and the helpful feedback I have incorporated all the requested changes:
After several iterations to iron out the CI regressions, all tests (including MSan and replication runs) are now passing successfuly Please let me know if there are any further modifications needed or if this looks good to go. |
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
Please update your patch to the latest 10.11 branch and factor in MDEV-38020 (#4701)’s init_ulonglongvar_from_file()
sql/rpl_mi.cc
Outdated
| semi_sync_reply_enabled(0) | ||
| { | ||
| char *tmp; | ||
| master_log_name[0] = 0; master_log_pos = 0; |
There was a problem hiding this comment.
I must’ve missed the need. What’s this addition doing here?
sql/slave.cc
Outdated
| if (error && error != MY_ERRNO_EDOM) | ||
| DBUG_RETURN(1); | ||
|
|
||
| while (my_isspace(&my_charset_latin1, *endptr)) | ||
| endptr++; | ||
| if (*endptr != '\0') | ||
| DBUG_RETURN(1); | ||
|
|
||
| if (error != MY_ERRNO_EDOM) | ||
| *var= (int) val; | ||
|
|
||
|
|
||
| DBUG_RETURN(0); | ||
| } | ||
| DBUG_RETURN(1); | ||
| DBUG_RETURN(0); |
There was a problem hiding this comment.
- Why would
MY_ERRNO_EDOMdefault the field with no error?…
Oh, I know: That’s MDEV-38020’s job. And it has been merged. - Why bother removing trailing spaces?
I wouldn’t, except for the line break. - If we’re reordering the flow, we might as well merge the two
DBUG_RETURN(0);s.
These comments also apply to init_floatvar_from_file() below.
ayush-jha123
force-pushed
the
fix-branch-10.11
branch
from
9d39733 to
7cfc61a
Compare
Modified init_intvar_from_file and init_floatvar_from_file to detect and reject trailing garbage in master.info numeric fields. Added a test case to verify the fix and ensure replication is correctly restored.
ayush-jha123
force-pushed
the
fix-branch-10.11
branch
from
7cfc61a to
4874b62
Compare
3 participants
I've updated this PR to target the 10.11 branch as requested and added a test case to verify the fix.
The issue was that
init_intvar_from_file
and
init_floatvar_from_file
were using atoi or simple sscanf, which would stop parsing at the first non-numeric character and leave the remaining "garbage" in the buffer. This garbage then corrupted the start of the next line.
I've changed the logic to use strtol and sscanf with %n to track exactly where parsing ends. Now, the code checks the rest of the line; if there's any non-whitespace characters left (like 3306abcdef), it returns an error instead of proceeding with a broken configuration.
The new MTR test main.mdev_38010 simulates a malformed master.info and confirms the server fails to load it, falling back to default values instead of accepting junk data.