MDEV-39123: Replace sprintf with snprintf in the InnoDB binlog code#4824
MDEV-39123: Replace sprintf with snprintf in the InnoDB binlog code#4824
Conversation
|
|
||
| static inline void | ||
| binlog_name_make_short(char *name_buf, uint64_t file_no) | ||
| binlog_name_make_short(char *name_buf, size_t sizeof_name_buf, uint64_t file_no) |
There was a problem hiding this comment.
sizeof_name_buf, really? maybe there is something more natural, and short, "buf_size"?
|
see bb-10.11-mac and Marko aproaches in the correspondent MDEV |
|
Georgi (Joro) Kodinov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| char filename[BINLOG_NAME_MAX_LEN]; | ||
| binlog_name_make_short(filename, file_no); | ||
| binlog_name_make_short(filename, sizeof(filename), file_no); | ||
| if (purge_info.nonpurge_reason) | ||
| sql_print_information("InnoDB: Binlog file %s could not be purged " | ||
| "because %s", | ||
| filename, purge_info.nonpurge_reason); |
There was a problem hiding this comment.
This function is a simple wrapper of snprintf(). What happens if more than BINLOG_NAME_MAX_LEN characters of input is available? Would the filename be terminated by \0, or could the subsequent sql_print_information() call exceed the bounds of the filename buffer. I tested this, because the documentation of snprintf(3) on my system is a little unclear:
#include <stdio.h>
int main()
{
char buf[2];
snprintf(buf, sizeof buf, "foo");
return printf("hello %s\n", buf);
}On my system, this would display hello f, so this appears to be fine.
There used to be an issue on Microsoft Windows here, but as noted in https://stackoverflow.com/questions/7706936/is-snprintf-always-null-terminating it was corrected some 10 years ago by implementing the standard snprintf() function.
There was a problem hiding this comment.
https://linux.die.net/man/3/snprintf says:
The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str.
| static_assert(BINLOG_NAME_MAX_LEN <= FN_REFLEN, | ||
| "FN_REFLEN too shot to hold InnoDB binlog name"); | ||
| binlog_name_make_short(name, file_no); | ||
| binlog_name_make_short(name, FN_REFLEN, file_no); |
There was a problem hiding this comment.
You could shoot at the static_assert message as well and correct the shot to short. Given that this branch includes cacaaeb, we could use the unary form of static_assert and reduce the source code line count by 1.
There was a problem hiding this comment.
There is a typo "too shot" in the static_assert message. You could just write
static_assert(BINLOG_NAME_MAX_LEN <= FN_REFLEN);because this branch uses a C++ standard version that supports the unary variant of static_assert.
There was a problem hiding this comment.
a bit outside of the scope of the fix, but OK. fixing.
| binlog_name_make_short(purge_info->nonpurge_filename, | ||
| sizeof(purge_info->nonpurge_filename), file_no); |
There was a problem hiding this comment.
The parentheses after sizeof are only needed when the argument is a name of a type. Here it is an expression, and therefore the parentheses are redundant.
There was a problem hiding this comment.
I will remove the parentheses. But, I was wondering (since this is not wrong as is), is there any coding style description that was violated? Or is this just a personal preference?
fa26f3c to
33c9d05
Compare
binlog code sprintf is deprecated (and triggers a deprecation warning) on some platforms, saying it needs to be replaced with snprintf. This is a good idea, since sprintf doesn't explicitly check the size of the buffer it is printing to. Fixed by adding an extra size argument to the helper function and making sure this extra argument is passed down from the caller to snprintf.
33c9d05 to
9de6beb
Compare
sprintf is deprecated (and triggers a deprecation warning) on some platforms, saying it needs to be replaced with snprintf.
This is a good idea, since sprintf doesn't explicitly check the size of the buffer it is printing to.
Fixed by adding an extra size argument to the helper function and making sure this extra argument is passed down from the caller to snprintf.