server icon indicating copy to clipboard operation
server copied to clipboard

Convert miscellaneous instances of `sprintf` to `snprintf`

Open tgross35 opened this issue 1 year ago • 4 comments

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

  • [x] 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

  • [x] 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

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

tgross35 avatar Feb 20 '24 07:02 tgross35

As in https://github.com/MariaDB/server/pull/3076#issuecomment-1954982335, 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.

dlenski avatar Feb 20 '24 20:02 dlenski

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

tgross35 avatar Feb 25 '24 05:02 tgross35

@LinuxJedi thanks, I changed the base to 10.5.

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

tgross35 avatar Apr 05 '24 01:04 tgross35

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.

knielsen avatar Apr 09 '24 17:04 knielsen