Convert instances of sprintf to snprintf in sql
#3075
Conversation
tgross35
force-pushed
the
remove-sprintf-sql
branch
from
ec58983
to
342d2d7
Compare
Per #2958 (comment), you've split this up into 3 smaller pieces in order to narrow down the source of the CI failures. |
sql/log_event.h
Outdated
| // TODO: use real value, struggling with imports here | ||
| snprintf(c+1, (buf + buf_len) - (c + 1), ",%lu", fmt); |
There was a problem hiding this comment.
What is the "real value" that you're struggling to obtain here?
sql/log_event_client.cc
Outdated
| char *str_end = to->str + to->length; | ||
| size_t add_to_len; | ||
|
|
||
| str += (to->length= sprintf(str, fmt_frag, 0)); | ||
| str += (to->length= snprintf(str, str_end-str, fmt_frag, 0)); |
There was a problem hiding this comment.
Do you think that str_end-str is easier to understand here than simply using to->length? I do not.
My preference would be something like…
size_t str_length = to->length;
…
snprintf(str, str_length, …)
sql/gcalc_slicescan.cc
Outdated
| lnbuf+= snprintf(buf + lnbuf, sizeof(buf) - lnbuf, | ||
| "%d\t", slice->thread); | ||
| lnbuf+= snprintf(buf + lnbuf, sizeof(buf) - lnbuf, | ||
| "%s\t", gcalc_ev_name(slice->event)); | ||
|
|
||
| lnbuf+= gcalc_pi_str(buf + lnbuf, slice->pi, "\t"); | ||
| lnbuf+= gcalc_pi_str(buf + lnbuf, sizeof(buf) - lnbuf, slice->pi, "\t"); | ||
| if (slice->is_bottom()) | ||
| lnbuf+= sprintf(buf+lnbuf, "bt\t"); | ||
| lnbuf+= snprintf(buf+lnbuf, sizeof(buf) - lnbuf, "bt\t"); | ||
| else | ||
| lnbuf+= gcalc_pi_str(buf+lnbuf, slice->next_pi, "\t"); | ||
| lnbuf+= gcalc_pi_str(buf + lnbuf, sizeof(buf) - lnbuf, | ||
| slice->next_pi, "\t"); |
There was a problem hiding this comment.
Did you mean to include these changes as part of this PR? They don't appear to be directly related to the goal of bounds-checking sprintf invocations.
It appears that you're actually adding new fields to each line printed here.
tgross35
force-pushed
the
remove-sprintf-sql
branch
3 times, most recently
from
8d1e1c0
to
865eb55
Compare
This adds an abstraction for the commonly used pointer-as-cursor interface, with checked operations. It is meant to simplify sequential operations of `sprintf` and `strcat` to a preallocated buffer. Signed-off-by: Trevor Gross <[email protected]>
Signed-off-by: Trevor Gross <[email protected]>
tgross35
force-pushed
the
remove-sprintf-sql
branch
from
865eb55
to
e776115
Compare
|
@LinuxJedi since you have been reviewing the other sprintf PRs, would you mind giving some thoughts on https://github.com/MariaDB/server/blob/e776115ebb723f4280573886363c93daa77ed342/include/bufcursor.h / https://github.com/MariaDB/server/blob/e776115ebb723f4280573886363c93daa77ed342/sql/bufcursor.c included as part of this PR? It is meant as an easy API to replace the pattern of I added this because that pattern is used frequently (the rest of this PR and sometimes in https://github.com/MariaDB/server/pull/3076/files). Switching directly to |
|
@tgross35 what would that provide us over using the |
Not enough to be special, I can update it to use that |
Remove use of
sprintfin favor ofsnprintfwhere indicated by deprecation warnings.Description
This is a portion of #2958
Release Notes
None needed
How can this PR be tested?
Compile with -Wdeprecated-declarations (I believe it is enabled by default for the relevant files) and use a version of libc that marks sprintf deprecated.
Basing the PR against the correct MariaDB version
PR quality check