Minimize unsafe C functions usage - replace strcat() and strcpy() - continued #2516

robinnewhouse · 2023-03-02T18:24:11Z

Description

567b681 introduced safe_strcpy() to minimize the use of C with
potentially unsafe memory overflow with strcpy() whose use is
discouraged.
Replace instances of strcpy() with safe_strcpy() where possible, limited
here to files in the sql/ directory.

How can this PR be tested?

All passing MTR tests still pass. Tested manually and in CI.

Basing the PR against the correct MariaDB version

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.

Copyright

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer
Amazon Web Services, Inc.

CLAassistant · 2023-03-02T18:24:43Z

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.}

robinnewhouse · 2023-03-06T20:25:44Z

Marked as WIP. Investigating failing MTR tests.

ottok · 2023-04-09T22:03:01Z

@arun-esh Would you like to review this? It is somewhat similar to your #2573.

robinnewhouse · 2023-05-02T01:07:35Z

Removed changes that were the cause of failing tests. Could this be reviewed?
cc @arun-esh

LinuxJedi

Some minor changes needed. I flagged a couple of the changes that go > 80 characters, but there are several more too.

LinuxJedi · 2023-05-19T12:27:10Z

sql/my_json_writer.cc

@@ -201,13 +201,13 @@ void Json_writer::add_size(longlong val)
  {
    /* Values less than 16MB are specified in KB for precision */
    len= my_snprintf(buf, sizeof(buf), "%lld", val/1024);
-    strcpy(buf + len, "Kb");
+    safe_strcpy(buf + len, sizeof(buf) - len, "Kb");


This one and the one below should probably all be wrapped up into the my_snprintf()

Done. let me know if this looks alright to you.

sql/rpl_parallel.cc

sql/semisync_master.cc

sql/sql_connect.cc

LinuxJedi

Looks good to me, I'll pass on to a second review.

LinuxJedi · 2024-02-21T16:30:47Z

sql/my_json_writer.cc

-    strcpy(buf + len, "Kb");
-    len+= 2;
-  }
+    len= my_snprintf(buf, sizeof(buf), "%lldKb", val/1024);


Something for another time (not this pull request). Lower case 'b' is the wrong terminology here.

Noted. Thanks for the correction.

cvicentiu

Minor style comment. Looks good to me. OK to push after the change.

cvicentiu · 2024-04-08T08:03:24Z

sql/hostname.cc

@@ -513,42 +513,48 @@ int ip_to_hostname(struct sockaddr_storage *ip_storage,

  DBUG_EXECUTE_IF("getnameinfo_error_noname",
                  {
-                    strcpy(hostname_buffer, "<garbage>");
+                    safe_strcpy(hostname_buffer, sizeof(hostname_buffer),
+                    "<garbage>");


Indentation is wrong here. The string should be aligned with the opening ( on the previous row. Same as the other DBUG_EXECUTE_IFs.

thanks @cvicentiu, just to avoid pushing commits back and forth I'll just ask here.
Is this the indentation you had in mind?

DBUG_EXECUTE_IF("getnameinfo_error_noname", { safe_strcpy(hostname_buffer, sizeof(hostname_buffer), "<garbage>"); err_code= EAI_NONAME; } );

robinnewhouse · 2024-04-16T22:05:36Z

Is this ok to push to 10.4 or would you like me to rebase to 10.5?

vuvova · 2024-04-17T08:09:19Z

I'd suggest 10.5, just to be on the safe side. We're doing the very last 10.4 within a couple of weeks.

robinnewhouse · 2024-04-17T19:36:58Z

@vuvova Rebased to 10.5
@cvicentiu Updated indentation.

vuvova

approving, based on #2516 (review)

Similar to MariaDB#2480. 567b681 introduced safe_strcpy() to minimize the use of C with potentially unsafe memory overflow with strcpy() whose use is discouraged. Replace instances of strcpy() with safe_strcpy() where possible, limited here to files in the `sql/` directory. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

robinnewhouse force-pushed the 10.4-safe-strcpy branch from d0134e5 to 354de40 Compare March 3, 2023 19:08

robinnewhouse changed the title ~~Minimize unsafe C functions usage - replace strcat() and strcpy() - continued~~ WIP: Minimize unsafe C functions usage - replace strcat() and strcpy() - continued Mar 6, 2023

ottok marked this pull request as draft March 18, 2023 21:07

robinnewhouse force-pushed the 10.4-safe-strcpy branch 2 times, most recently from 5f70ae2 to 9f8f14a Compare March 22, 2023 00:24

robinnewhouse marked this pull request as ready for review March 22, 2023 00:24

robinnewhouse force-pushed the 10.4-safe-strcpy branch 2 times, most recently from 7fbddc3 to b88b393 Compare April 3, 2023 21:48

robinnewhouse changed the title ~~WIP: Minimize unsafe C functions usage - replace strcat() and strcpy() - continued~~ Minimize unsafe C functions usage - replace strcat() and strcpy() - continued Apr 9, 2023

robinnewhouse force-pushed the 10.4-safe-strcpy branch 2 times, most recently from 7130ec6 to 2d73c86 Compare May 4, 2023 19:27

LinuxJedi requested changes May 19, 2023

View reviewed changes

robinnewhouse force-pushed the 10.4-safe-strcpy branch 2 times, most recently from ccd3731 to 4669259 Compare July 25, 2023 21:21

robinnewhouse requested a review from LinuxJedi January 30, 2024 18:38

robinnewhouse force-pushed the 10.4-safe-strcpy branch from 4669259 to 2192f94 Compare January 30, 2024 18:38

robinnewhouse force-pushed the 10.4-safe-strcpy branch from 2192f94 to 7b18497 Compare February 7, 2024 18:36

LinuxJedi approved these changes Feb 21, 2024

View reviewed changes

LinuxJedi requested a review from cvicentiu February 21, 2024 16:32

cvicentiu requested changes Apr 8, 2024

View reviewed changes

robinnewhouse force-pushed the 10.4-safe-strcpy branch from 7b18497 to 522afb8 Compare April 17, 2024 19:35

robinnewhouse changed the base branch from 10.4 to 10.5 April 17, 2024 19:36

robinnewhouse requested a review from cvicentiu April 17, 2024 19:37

vuvova approved these changes Apr 25, 2024

View reviewed changes

cvicentiu approved these changes Apr 25, 2024

View reviewed changes

LinuxJedi force-pushed the 10.4-safe-strcpy branch from 522afb8 to 13c023a Compare May 17, 2024 11:56

LinuxJedi enabled auto-merge (rebase) May 17, 2024 11:56

LinuxJedi merged commit dc38d8e into MariaDB:10.5 May 17, 2024
12 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimize unsafe C functions usage - replace strcat() and strcpy() - continued #2516

Minimize unsafe C functions usage - replace strcat() and strcpy() - continued #2516

robinnewhouse commented Mar 2, 2023

CLAassistant commented Mar 2, 2023 •

edited

robinnewhouse commented Mar 6, 2023

ottok commented Apr 9, 2023

robinnewhouse commented May 2, 2023

LinuxJedi left a comment

LinuxJedi May 19, 2023

robinnewhouse Jul 25, 2023

LinuxJedi left a comment

LinuxJedi Feb 21, 2024

robinnewhouse Feb 21, 2024

cvicentiu left a comment

cvicentiu Apr 8, 2024

robinnewhouse Apr 16, 2024

robinnewhouse commented Apr 16, 2024

vuvova commented Apr 17, 2024

robinnewhouse commented Apr 17, 2024 •

edited

vuvova left a comment

Minimize unsafe C functions usage - replace strcat() and strcpy() - continued #2516

Minimize unsafe C functions usage - replace strcat() and strcpy() - continued #2516

Conversation

robinnewhouse commented Mar 2, 2023

Description

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

Copyright

CLAassistant commented Mar 2, 2023 • edited

robinnewhouse commented Mar 6, 2023

ottok commented Apr 9, 2023

robinnewhouse commented May 2, 2023

LinuxJedi left a comment

Choose a reason for hiding this comment

LinuxJedi May 19, 2023

Choose a reason for hiding this comment

robinnewhouse Jul 25, 2023

Choose a reason for hiding this comment

LinuxJedi left a comment

Choose a reason for hiding this comment

LinuxJedi Feb 21, 2024

Choose a reason for hiding this comment

robinnewhouse Feb 21, 2024

Choose a reason for hiding this comment

cvicentiu left a comment

Choose a reason for hiding this comment

cvicentiu Apr 8, 2024

Choose a reason for hiding this comment

robinnewhouse Apr 16, 2024

Choose a reason for hiding this comment

robinnewhouse commented Apr 16, 2024

vuvova commented Apr 17, 2024

robinnewhouse commented Apr 17, 2024 • edited

vuvova left a comment

Choose a reason for hiding this comment

CLAassistant commented Mar 2, 2023 •

edited

robinnewhouse commented Apr 17, 2024 •

edited