Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MDEV-35299 #3613

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from
Open

MDEV-35299 #3613

wants to merge 1 commit into from

Conversation

dmitryshulga
Copy link
Contributor

@dmitryshulga dmitryshulga commented Nov 6, 2024 •
edited
Loading

  • The Jira issue number for this PR is: MDEV-35299

Description

Support compilation of the server on MacOS with XCode 14.2

How can this PR be tested?

Try to build server on MacOS with XCode 14.2

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main 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.

@CLAassistant
Copy link

CLAassistant commented Nov 6, 2024 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@dmitryshulga dmitryshulga changed the base branch from main to 10.5 November 7, 2024 04:32
grooverdan
grooverdan reviewed Nov 7, 2024
View reviewed changes
plugin/type_inet/sql_type_inet.cc
@@ -493,8 +493,13 @@ size_t Inet6::to_string(char *dst, size_t dstsize) const
//
// If it is not the last field, append closing ':'.

p += sprintf(p, "%x", ipv6_words[i]);
int ret= snprintf(p, dstsize_available, "%x", ipv6_words[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Vista EOL this could be inet_ntop now. Btw good pattern, a recent security conference shows the dangers of p+= snprintf(p because it doesn't mean that the full length was written to the buffer.

Is it really safe to return without a SQL warning pushed?

sql/log_event.cc Outdated
@@ -1409,7 +1409,14 @@ code_name(int code)
case Q_MASTER_DATA_WRITTEN_CODE: return "Q_MASTER_DATA_WRITTEN_CODE";
case Q_HRNOW: return "Q_HRNOW";
}
#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notably never reached as CHECK_SPACE, the only caller of code_name always has a valid code. Changing to a snprint would be less verbose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the exact same comment in MDEV-35299 a couple of days ago. Basically, after a proper fix, git grep -w sprintf should not return anything.

sql/log_event.h Outdated
@@ -3177,22 +3188,11 @@ inline char *serialize_xid(char *buf, long fmt, long gln, long bln,
c+= 2;
}
c[0]= '\'';
sprintf(c+1, ",%lu", fmt);
snprintf(c + 1, ser_buf_size - 1, ",%lu", fmt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically ser_buf_size - 1 - (2 + (gln + bln) * 2 + 4 + 1)

sql/log_event_client.cc Outdated
sprintf(tmp, "%-.20g", dbl); /* strmake doesn't support %-20g */
#ifdef __clang__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think compilers could determine if this is safe with a sizeof rather than warning suppression.

Could expand on safe vs not safe in commit message.

@dmitryshulga dmitryshulga force-pushed the 10.5-MDEV-35299 branch 2 times, most recently from 01b027b to dbb581e Compare November 12, 2024 14:44
dr-m
dr-m reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@dr-m dr-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that all occurrences of sprintf() must be replaced. This should be easy to check with git grep -w sprintf.

storage/innobase/ut/ut0ut.cc Outdated
Comment on lines 107 to 111
char* buf) /*!< in: buffer where to sprintf */
char* buf, /*!< in: buffer where to sprintf */
size_t len)
{
#ifdef _WIN32
SYSTEMTIME cal_tm;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter len is undocumented and not used #ifdef _WIN32.

dr-m
dr-m reviewed Nov 13, 2024
View reviewed changes
sql/password.c Outdated
Comment on lines 122 to 128
{
ulong hash_res[2];
hash_password(hash_res, password, (uint) pass_len);
#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
#endif
sprintf(to, "%08lx%08lx", hash_res[0], hash_res[1]);
#ifdef __clang__
#pragma clang diagnostic pop
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least one caller (execute_commands() in client/mysqladmin.cc) is invoking this with to pointing to a buffer of 64 bytes. Based on the format expression, the length should be 16 bytes. Based on the ulong data type, whose size is 64 bits on LP64 systems and 32 bits elsewhere (including the LLP64 Microsoft Windows), the length could be up to 32 bytes. It seems to me that the ulong should be replaced with uint32_t, because SCRAMBLE_LENGTH_323 is defined as 8.

In Item_func_password::val_str_ascii(), the argument is a pointer to the buffer tmp_value[SCRAMBLED_PASSWORD_CHAR_LENGTH+1]. It seems that this is the length that should be passed to snprintf here, and should be mentioned in the function documentation as well.

sql/password.c Outdated
Comment on lines 278 to 288
void make_password_from_salt_323(char *to, const ulong *salt)
{
#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
#endif
sprintf(to,"%08lx%08lx", salt[0], salt[1]);
#ifdef __clang__
#pragma clang diagnostic pop
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems to have been orphaned in 1ff476b. Is it really needed?

@dmitryshulga dmitryshulga force-pushed the 10.5-MDEV-35299 branch 4 times, most recently from 13f17b0 to c13f5ca Compare November 14, 2024 14:34
@dmitryshulga
MDEV-35299: Server build failure on MacOS Monterey
689a190 
Build failure was caused by using the sprintf C API function
that is marked as deprecated in MacOS Monterey. Since building
of server is performaed with the compiler flag -Werror
it results in build failure.

To fix the issue, every use of the C API function sprintf
has been replaced with snprintf throughout the source code.
Source code of the storage engine CONNECT wasn't modified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-m dr-m dr-m left review comments

@grooverdan grooverdan grooverdan left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmitryshulga @CLAassistant @grooverdan @dr-m