-
-
Notifications
You must be signed in to change notification settings - Fork 7k
lib: strcopy() instead of strcpy() [PROPOSAL] #20067
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.
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Move to curlx perhaps? It'd allow using it from src (just 2 calls there) and tests (almost 20). This raises a naming question: it could live as curlx_strcopy there, with an alias of strcopy for lib. Either way, it'd be useful to support it outside lib. It'd allow dropping the clang-tidy |
Yes, agreed. I just paused at this point as I figured this shows the idea and concept to work a discussion starter. |
7dc8033 to
d20cf3a
Compare
5f53aa5 to
2f227bf
Compare
|
augment review |
|
Potentially we should make the function not touch the target buffer at all if the copy can't be done... |
🤖 Augment PR SummarySummary: Introduces a new internal helper Changes:
Technical Notes: 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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 proposes introducing curlx_strcopy() as a safer alternative to strcpy(). The new function requires both the destination buffer size and source string length as parameters, making buffer overflows harder to introduce.
Key changes:
- New
curlx/strcopy.candcurlx/strcopy.hfiles implementing the safer string copy function - Updates to build files (Makefile.inc) across lib/, src/, and test directories to include the new files
- Replacement of
strcpy()calls withcurlx_strcopy()across multiple source files, with callers now explicitly passing buffer sizes and string lengths
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/curlx/strcopy.h | Header file defining the curlx_strcopy function signature |
| lib/curlx/strcopy.c | Implementation of safe string copy with size checking |
| lib/curlx/inet_ntop.c | Replaces strcpy with curlx_strcopy and removes explicit error handling |
| lib/curlx/strerr.c | Updates Winsock error handling to use curlx_strcopy |
| lib/curlx/winapi.c | Updates Windows API error handling to use curlx_strcopy |
| lib/curlx/curlx.h | Adds include for strcopy.h |
| lib/Makefile.inc | Adds strcopy.c and strcopy.h to build system |
| lib/ws.c | Updates WebSocket key generation to use curlx_strcopy |
| lib/vtls/wolfssl.c | Updates wolfSSL error string handling |
| lib/vtls/vtls.c | Updates multi-SSL version string handling |
| lib/vtls/openssl.c | Updates OpenSSL error string handling |
| lib/vquic/curl_osslq.c | Updates QUIC OpenSSL error string handling |
| lib/urlapi.c | Fixes buffer size in IPv6 parsing (hlen + 1) |
| lib/tftp.c | Refactors tftp_option_add to use index-based addressing |
| lib/strerror.c | Updates SSPI error handling to use curlx_strcopy |
| lib/strdup.h | Removes trailing blank line |
| lib/smb.c | Updates SMB protocol path handling |
| lib/progress.c | Updates progress meter formatting functions with buffer size parameters |
| lib/mime.c | Updates quoted-printable encoding soft line break handling |
| lib/imap.c | Updates IMAP response tag initialization |
| lib/hsts.c | Updates HSTS expiry string handling |
| lib/hostip.c | Updates localhost canonical name handling |
| lib/curl_trc.c | Updates error buffer handling |
| lib/curl_gssapi.c | Updates GSSAPI credential handling |
| lib/curl_gethostname.c | Updates hostname handling with environment override |
| lib/content_encoding.c | Updates content encoding string building |
| src/tool_progress.c | Updates progress display time and data formatting with buffer sizes |
| src/tool_main.c | Updates memory debug filename handling |
| src/Makefile.inc | Adds strcopy files to build system |
| tests/server/Makefile.inc | Adds strcopy.c to test server build |
| tests/libtest/Makefile.inc | Adds strcopy.c to libtest build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This function REQUIRES the size of the target buffer as well as the length of the source string. Meant to make it harder to do a bad strcpy().
4382e54 to
f249199
Compare
There was a problem hiding this comment.
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 30 out of 30 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This function REQUIRES the size of the target buffer as well as the length of the source string. Meant to make it harder to do a bad strcpy().
Discussion item. A proposal. How do we make it better?