Convert miscellaneous instances of `sprintf` to `snprintf` #3077

tgross35 · 2024-02-20T07:46:09Z

Remove use of sprintf in favor of snprintf where indicated by deprecation warnings.

The Jira issue number for this PR is: MDEV-______

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

This is a new feature and the PR is based against the latest MariaDB development branch.
This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

client/mysqladmin.cc

plugin/type_inet/sql_type_inet.cc

dlenski · 2024-02-20T20:18:44Z

As in #3076 (comment), it appears that the buildbot/CI failures on this PR are not due to changes in the PR, but rather they are "preexisting" failures on the upstream source branch.

tgross35 · 2024-02-25T05:50:53Z

This one should be ready for another look when you get the chance @dlenski

dlenski · 2024-02-26T18:48:21Z

plugin/type_inet/sql_type_inet.cc

+ int res= snprintf(p, dstsize_available, "%x", ipv6_words[i]);
+ DBUG_ASSERT(res >= 0);


I don't think the return value of snprintf tells you anything useful here. https://linux.die.net/man/3/snprintf#:~:text=Concerning%20the%20return%20value%20of,large%20enough.

It can return -1 if printing to a file fails or if there is an error in the format string. No chance of that as-is, but figure it's a trivial check to make sure we never accidentally increment the available size.

I only added this check in cases where we actually use the result, though it wouldn't be a terrible thing (if annoying) to have elsewhere

dlenski · 2024-02-26T18:48:59Z

plugin/type_inet/sql_type_inet.cc

+ dstsize_available= dstend - p;
+ int res= snprintf(p, dstsize_available, "%x", ipv6_words[i]);
+ DBUG_ASSERT(res >= 0);
+ p += res;


Should dstsize_available be decreased here as well? E.g. p += res; dstsize_available -= res;

Actually it doesn't have to be set at all since it is only read once per block, I removed line 502 here. The other blocks do not update dstsize_available after doing the pointer math.

Hm, https://github.com/MariaDB/server/compare/0d716561e096ffb2c71f83c1e5f3131cf7a02e8d..d457f92c44acb8f62fceb649a94672972b511102 failed CI which doesn't make sense. Maybe a fluke, will retry.

Must have just been flaky, all required tests pass now

LinuxJedi

I don't see any initial problems with this. Many thanks! Although this could go against 10.5 instead, cover more versions.

Remove use of `sprintf` in favor of `snprintf` where indicated by deprecation warnings. Signed-off-by: Trevor Gross <[email protected]>

tgross35 · 2024-04-05T01:44:48Z

@LinuxJedi thanks, I changed the base to 10.5.

Side question, about how often do syncs among the versions happen?

knielsen · 2024-04-08T20:14:16Z

client/mysqladmin.cc

@@ -1428,10 +1428,12 @@ static void usage(void)
 version Get version info from server");
 }

+#define SHOW_MSG_LEN (FN_REFLEN + 20)


Why introduce this #define SHOW_MSG_LEN? It only makes it harder to understand the code, that the right size is being passed to snprintf() (as opposed to just sizeof()), and that the buffer is large enough that it will not be truncated and corrupt the command to be contructed.

You are right, I was thinking this existed elsewhere too somehow

Actually this was by request to remove the magic values #3077 (comment)

To be fair to @tgross35, our coding standards document is unclear for this specific instance. We should maybe update that section. In other projects, this would be the way to do things, maybe with a comment.

knielsen · 2024-04-08T20:16:10Z

client/mysqladmin.cc

@@ -1428,10 +1428,12 @@ static void usage(void)
 version Get version info from server");
 }

+#define SHOW_MSG_LEN (FN_REFLEN + 20)
+#define UINPUT_BUF_LEN 10


Why introduce this #define UINPUT_BUF_LEN? This is unrelated to sprintf/snprintf(), should be done as a separate patch (or why do it at all, the code is simpler without the #define, it's only used in one place)

knielsen · 2024-04-08T20:18:44Z

mysql-test/lib/My/SafeProcess/safe_process.cc

+ snprintf(safe_process_name, SAFE_PROCESS_NAME_LEN,
+ "safe_process[%ld]", (long) own_pid);


Again, don't introduce SAFE_PROCESS_NAME_LEN. It only makes it harder to see that the buffer size is correct for the string being constructed, and that the correct length is passed to sprintf() (as opposed to using sizeof())

knielsen · 2024-04-09T17:35:45Z

Also what about testing? The patch contains no test cases.

The mysqladmin changes have a restriction on the database name of FN_REFLEN. It needs to be tested that database names up to this length work, and longer (eg. FN_REFLEN+1) throw an error higher in the code, so the snprintf() doesn't silently truncate the name and send a corrupted command to the server.

If this is already covered by existing tests, it would be good to mention this in the commit comment.

tgross35 mentioned this pull request Feb 20, 2024

Convert core instances of sprintf to snprintf #2958

Closed

5 tasks

dlenski reviewed Feb 20, 2024

View reviewed changes

client/mysqladmin.cc Outdated Show resolved Hide resolved

plugin/type_inet/sql_type_inet.cc Outdated Show resolved Hide resolved

tgross35 force-pushed the remove-sprintf-client-backup branch 3 times, most recently from 208739c to 0d71656 Compare February 25, 2024 03:29

dlenski reviewed Feb 26, 2024

View reviewed changes

tgross35 force-pushed the remove-sprintf-client-backup branch 4 times, most recently from e378995 to e29c8a0 Compare February 27, 2024 23:14

tgross35 force-pushed the remove-sprintf-client-backup branch from e29c8a0 to 08b950f Compare March 8, 2024 19:38

LinuxJedi approved these changes Apr 4, 2024

View reviewed changes

tgross35 force-pushed the remove-sprintf-client-backup branch from 08b950f to cf87f76 Compare April 5, 2024 01:25

Convert miscellaneous instances of sprintf to snprintf

bfda187

Remove use of `sprintf` in favor of `snprintf` where indicated by deprecation warnings. Signed-off-by: Trevor Gross <[email protected]>

tgross35 force-pushed the remove-sprintf-client-backup branch from cf87f76 to bfda187 Compare April 5, 2024 01:41

tgross35 changed the base branch from 11.5 to 10.5 April 5, 2024 01:41

knielsen requested changes Apr 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert miscellaneous instances of `sprintf` to `snprintf` #3077

Convert miscellaneous instances of `sprintf` to `snprintf` #3077

tgross35 commented Feb 20, 2024

dlenski commented Feb 20, 2024

tgross35 commented Feb 25, 2024

dlenski Feb 26, 2024

tgross35 Feb 26, 2024 •

edited

dlenski Feb 26, 2024

tgross35 Feb 26, 2024

tgross35 Feb 26, 2024

tgross35 Feb 28, 2024

LinuxJedi left a comment •

edited

tgross35 commented Apr 5, 2024

knielsen Apr 8, 2024

tgross35 Apr 8, 2024

tgross35 Apr 8, 2024

LinuxJedi Apr 12, 2024

knielsen Apr 8, 2024

knielsen Apr 8, 2024

knielsen commented Apr 9, 2024

		int res= snprintf(p, dstsize_available, "%x", ipv6_words[i]);
		DBUG_ASSERT(res >= 0);

		snprintf(safe_process_name, SAFE_PROCESS_NAME_LEN,
		"safe_process[%ld]", (long) own_pid);

Convert miscellaneous instances of sprintf to snprintf #3077

Are you sure you want to change the base?

Convert miscellaneous instances of sprintf to snprintf #3077

Conversation

tgross35 commented Feb 20, 2024

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

dlenski commented Feb 20, 2024

tgross35 commented Feb 25, 2024

Choose a reason for hiding this comment

tgross35 Feb 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinuxJedi left a comment • edited

Choose a reason for hiding this comment

tgross35 commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knielsen commented Apr 9, 2024

Convert miscellaneous instances of `sprintf` to `snprintf` #3077

Convert miscellaneous instances of `sprintf` to `snprintf` #3077

tgross35 Feb 26, 2024 •

edited

LinuxJedi left a comment •

edited