Skip to content

os400sys: replace strcpy() with memcpy() - #20089

Closed
vszakats wants to merge 4 commits into
curl:masterfrom
vszakats:os400strcpy
Closed

os400sys: replace strcpy() with memcpy()#20089
vszakats wants to merge 4 commits into
curl:masterfrom
vszakats:os400strcpy

Conversation

@vszakats

@vszakats vszakats commented Dec 24, 2025

Copy link
Copy Markdown
Member

Source and target are the same size, null-terminator is already present
in the target buffer.


  • add comments about null-terminator.

/cc @monnerat

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR attempts to replace strcpy() with memcpy() for improved performance and safety in three LDAP-related functions in the OS400 system code. The changes also remove explicit null-terminator assignments based on the assumption that they are already present in the target buffer.

  • Replace strcpy() calls with memcpy() in three functions
  • Remove explicit null-terminator assignments after character set conversion
  • Apply changes to Curl_ldap_get_dn_a, Curl_ldap_first_attribute_a, and Curl_ldap_next_attribute_a

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/OS400/os400sys.c Outdated
Comment thread packages/OS400/os400sys.c Outdated
Comment thread packages/OS400/os400sys.c Outdated
Comment thread packages/OS400/os400sys.c
Comment thread packages/OS400/os400sys.c
Comment thread packages/OS400/os400sys.c

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/OS400/os400sys.c
overwrite the EBCDIC buffer with ASCII to return it. */

strcpy(cp, cp2);
memcpy(cp, cp2, i);

Copilot AI Dec 24, 2025

Copy link

Choose a reason for hiding this comment

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

The memcpy call copies only i bytes from cp2 to cp, which does not include the null-terminator. The original cp buffer obtained from ldap_get_dn had a null-terminator at position i, but after this memcpy operation overwrites it without restoring it, the returned string will not be null-terminated. This can lead to buffer overruns and undefined behavior when the returned string is used. A null-terminator needs to be explicitly added after the memcpy operation.

Copilot uses AI. Check for mistakes.

@vszakats vszakats Dec 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i contains the length without the terminating null, cp has its original terminating null, and memcpy copies back the number of character without terminating null, leaving the one already there. This looks correct to me.

Comment thread packages/OS400/os400sys.c
Comment thread packages/OS400/os400sys.c
@monnerat

Copy link
Copy Markdown
Contributor

Yes, this will do the same and is more efficient.
Maybe comment the null-terminator consideration, as it does not appear explicitly in the code.
Thanks for this.
Approved.

@vszakats

vszakats commented Dec 24, 2025

Copy link
Copy Markdown
Member Author

Thanks for the review @monnerat! I merged with comments added.

@vszakats
vszakats deleted the os400strcpy branch December 24, 2025 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants