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.

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

use strndup/memdup instead of malloc, memcpy and null-terminate etc #12453

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 2 additions & 4 deletions lib/bufref.c
Expand Up @@ -25,6 +25,7 @@
#include "curl_setup.h"
#include "urldata.h"
#include "bufref.h"
#include "strdup.h"

#include "curl_memory.h"
#include "memdebug.h"
Expand Down Expand Up @@ -116,12 +117,9 @@ CURLcode Curl_bufref_memdup(struct bufref *br, const void *ptr, size_t len)
DEBUGASSERT(len <= CURL_MAX_INPUT_LENGTH);

if(ptr) {
cpy = malloc(len + 1);
cpy = Curl_strndup(ptr, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong: the function is supposed to duplicate the given byte count, but this will stop at the first null byte.
The original code behaves more like memdup, but appends a null byte as a sentinel in case the data is effectively a string.
Maybe we should have a such a generic dual-use duplication function.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, Curl_strndup() does not stop at the first null byte (any more). That is exactly what I mentioned above in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad! I've been abused by the Curl_strndup() original code and the usage semantics associated with this name.
Sorry for the noise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been pondering on maybe renaming this function just to make it less likely that one would think that it has this functionality...

Copy link
Contributor

Choose a reason for hiding this comment

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

... one would think that it has this functionality...

This happened to me: will probably affect others.

if(!cpy)
return CURLE_OUT_OF_MEMORY;
if(len)
memcpy(cpy, ptr, len);
cpy[len] = '\0';
}

Curl_bufref_set(br, cpy, len, curl_free);
Expand Down
4 changes: 1 addition & 3 deletions lib/cookie.c
Expand Up @@ -821,10 +821,8 @@ Curl_cookie_add(struct Curl_easy *data,
endslash = memrchr(path, '/', (queryp - path));
if(endslash) {
size_t pathlen = (endslash-path + 1); /* include end slash */
co->path = malloc(pathlen + 1); /* one extra for the zero byte */
co->path = Curl_strndup(path, pathlen);
if(co->path) {
memcpy(co->path, path, pathlen);
co->path[pathlen] = 0; /* null-terminate */
co->spath = sanitize_cookie_path(co->path);
if(!co->spath)
badcookie = TRUE; /* out of memory bad */
Expand Down
4 changes: 1 addition & 3 deletions lib/formdata.c
Expand Up @@ -779,11 +779,9 @@ static CURLcode setname(curl_mimepart *part, const char *name, size_t len)

if(!name || !len)
return curl_mime_name(part, name);
zname = malloc(len + 1);
zname = Curl_strndup(name, len);
if(!zname)
return CURLE_OUT_OF_MEMORY;
memcpy(zname, name, len);
zname[len] = '\0';
res = curl_mime_name(part, zname);
free(zname);
return res;
Expand Down
7 changes: 3 additions & 4 deletions lib/ftp.c
Expand Up @@ -72,6 +72,7 @@
#include "warnless.h"
#include "http_proxy.h"
#include "socks.h"
#include "strdup.h"
/* The last 3 #include files should be in this order */
#include "curl_printf.h"
#include "curl_memory.h"
Expand Down Expand Up @@ -4177,13 +4178,12 @@ CURLcode ftp_parse_url_path(struct Curl_easy *data)
return CURLE_OUT_OF_MEMORY;
}

ftpc->dirs[0] = calloc(1, dirlen + 1);
ftpc->dirs[0] = Curl_strndup(rawPath, dirlen);
if(!ftpc->dirs[0]) {
free(rawPath);
return CURLE_OUT_OF_MEMORY;
}

strncpy(ftpc->dirs[0], rawPath, dirlen);
ftpc->dirdepth = 1; /* we consider it to be a single dir */
fileName = slashPos + 1; /* rest is file name */
}
Expand Down Expand Up @@ -4222,12 +4222,11 @@ CURLcode ftp_parse_url_path(struct Curl_easy *data)
CWD requires a parameter and a non-existent parameter a) doesn't
work on many servers and b) has no effect on the others. */
if(compLen > 0) {
char *comp = calloc(1, compLen + 1);
char *comp = Curl_strndup(curPos, compLen);
if(!comp) {
free(rawPath);
return CURLE_OUT_OF_MEMORY;
}
strncpy(comp, curPos, compLen);
ftpc->dirs[ftpc->dirdepth++] = comp;
}
curPos = slashPos + 1;
Expand Down
10 changes: 1 addition & 9 deletions lib/http.c
Expand Up @@ -297,7 +297,6 @@ char *Curl_copy_header_value(const char *header)
{
const char *start;
const char *end;
char *value;
size_t len;

/* Find the end of the header name */
Expand Down Expand Up @@ -330,14 +329,7 @@ char *Curl_copy_header_value(const char *header)
/* get length of the type */
len = end - start + 1;

value = malloc(len + 1);
if(!value)
return NULL;

memcpy(value, start, len);
value[len] = 0; /* null-terminate */

return value;
return Curl_strndup(start, len);
}

#ifndef CURL_DISABLE_HTTP_AUTH
Expand Down
6 changes: 2 additions & 4 deletions lib/md4.c
Expand Up @@ -194,11 +194,9 @@ static int MD4_Init(MD4_CTX *ctx)
static void MD4_Update(MD4_CTX *ctx, const void *data, unsigned long size)
{
if(!ctx->data) {
ctx->data = malloc(size);
if(ctx->data) {
memcpy(ctx->data, data, size);
ctx->data = Curl_memdup(data, size);
if(ctx->data)
ctx->size = size;
}
}
}

Expand Down
7 changes: 3 additions & 4 deletions lib/pingpong.c
Expand Up @@ -36,6 +36,7 @@
#include "pingpong.h"
#include "multiif.h"
#include "vtls/vtls.h"
#include "strdup.h"

/* The last 3 #include files should be in this order */
#include "curl_printf.h"
Expand Down Expand Up @@ -424,10 +425,8 @@ CURLcode Curl_pp_readresp(struct Curl_easy *data,

if(clipamount) {
pp->cache_size = clipamount;
pp->cache = malloc(pp->cache_size);
if(pp->cache)
memcpy(pp->cache, pp->linestart_resp, pp->cache_size);
else
pp->cache = Curl_memdup(pp->linestart_resp, pp->cache_size);
if(!pp->cache)
return CURLE_OUT_OF_MEMORY;
}
if(restart) {
Expand Down
6 changes: 2 additions & 4 deletions lib/rtsp.c
Expand Up @@ -922,7 +922,7 @@ CURLcode Curl_rtsp_parseheader(struct Curl_easy *data, char *header)

/* If the Session ID is set, then compare */
if(strlen(data->set.str[STRING_RTSP_SESSION_ID]) != idlen ||
strncmp(start, data->set.str[STRING_RTSP_SESSION_ID], idlen) != 0) {
strncmp(start, data->set.str[STRING_RTSP_SESSION_ID], idlen)) {
failf(data, "Got RTSP Session ID Line [%s], but wanted ID [%s]",
start, data->set.str[STRING_RTSP_SESSION_ID]);
return CURLE_RTSP_SESSION_ERROR;
Expand All @@ -934,11 +934,9 @@ CURLcode Curl_rtsp_parseheader(struct Curl_easy *data, char *header)
*/

/* Copy the id substring into a new buffer */
data->set.str[STRING_RTSP_SESSION_ID] = malloc(idlen + 1);
data->set.str[STRING_RTSP_SESSION_ID] = Curl_strndup(start, idlen);
if(!data->set.str[STRING_RTSP_SESSION_ID])
return CURLE_OUT_OF_MEMORY;
memcpy(data->set.str[STRING_RTSP_SESSION_ID], start, idlen);
(data->set.str[STRING_RTSP_SESSION_ID])[idlen] = '\0';
}
}
else if(checkprefix("Transport:", header)) {
Expand Down
7 changes: 3 additions & 4 deletions lib/socks_gssapi.c
Expand Up @@ -35,6 +35,7 @@
#include "timeval.h"
#include "socks.h"
#include "warnless.h"
#include "strdup.h"

/* The last 3 #include files should be in this order */
#include "curl_printf.h"
Expand Down Expand Up @@ -139,10 +140,9 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf,
/* prepare service name */
if(strchr(serviceptr, '/')) {
service.length = serviceptr_length;
service.value = malloc(service.length);
service.value = Curl_memdup(serviceptr, service.length);
if(!service.value)
return CURLE_OUT_OF_MEMORY;
memcpy(service.value, serviceptr, service.length);

gss_major_status = gss_import_name(&gss_minor_status, &service,
(gss_OID) GSS_C_NULL_OID, &server);
Expand Down Expand Up @@ -387,12 +387,11 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf,
}
else {
gss_send_token.length = 1;
gss_send_token.value = malloc(1);
gss_send_token.value = Curl_memdup(&gss_enc, 1);
if(!gss_send_token.value) {
gss_delete_sec_context(&gss_status, &gss_context, NULL);
return CURLE_OUT_OF_MEMORY;
}
memcpy(gss_send_token.value, &gss_enc, 1);

gss_major_status = gss_wrap(&gss_minor_status, gss_context, 0,
GSS_C_QOP_DEFAULT, &gss_send_token,
Expand Down
8 changes: 2 additions & 6 deletions lib/strdup.c
Expand Up @@ -104,18 +104,14 @@ void *Curl_memdup(const void *src, size_t length)
* Curl_strndup(source, length)
*
* Copies the 'source' string to a newly allocated buffer (that is returned).
* Copies not more than 'length' bytes (up to a null terminator) then adds a
* null terminator.
* Copies 'length' bytes then adds a null terminator.
*
* Returns the new pointer or NULL on failure.
*
***************************************************************************/
void *Curl_strndup(const char *src, size_t length)
{
char *buf = memchr(src, '\0', length);
if(buf)
length = buf - src;
buf = malloc(length + 1);
char *buf = malloc(length + 1);
if(!buf)
return NULL;
Comment on lines 115 to 116
Copy link
Member

Choose a reason for hiding this comment

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

I want to remove the memchr stuff again from strndup() because I think it potentially breaks several of these strndup cases.

How is it breaking? If we don't check if the length of src is shorter than the passed in length then the memcpy can read past the end of src. If the full passed in length needs to be allocated then it could be adjusted after the allocation but memchr would need to be used:

Suggested change
if(!buf)
return NULL;
char *end;
if(!buf)
return NULL;
end = memchr(src, '\0', length);
if(end)
length = end - src;

Copy link
Member Author

Choose a reason for hiding this comment

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

Because nowhere in the code does it assume that it should only add the substring up to a null terminator. They all want the specified subtring allocated, with a null terminator so that it can be treated as a string.

In some cases the substring might contain a null byte and then the added null terminator is of course unnecessary but that's how the code already works. If the memchr() logic remains , this use case breaks.

Copy link
Member

@jay jay Dec 6, 2023

Choose a reason for hiding this comment

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

Can you give me an example? I still don't get it.

edit: If you mean the rest of the memory should be cleared then use calloc instead of malloc?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want:

  1. alloc N bytes + one byte null termation
  2. copy N bytes (not to the nearest null byte) to the target memory

Not one of these cases want the copy to stop at the first null byte.

See the sectransp.c, ntlm_sspi.c and mbedtls.c changes. Scanning for null bytes in there is just wasteful or at worst wrong.

memcpy(buf, src, length);
Expand Down
6 changes: 3 additions & 3 deletions lib/vauth/ntlm.c
Expand Up @@ -44,6 +44,7 @@
#include "warnless.h"
#include "rand.h"
#include "vtls/vtls.h"
#include "strdup.h"

#define BUILDING_CURL_NTLM_MSGS_C
#include "vauth/vauth.h"
Expand Down Expand Up @@ -184,11 +185,10 @@ static CURLcode ntlm_decode_type2_target(struct Curl_easy *data,
}

free(ntlm->target_info); /* replace any previous data */
ntlm->target_info = malloc(target_info_len);
ntlm->target_info = Curl_memdup(&type2[target_info_offset],
target_info_len);
if(!ntlm->target_info)
return CURLE_OUT_OF_MEMORY;

memcpy(ntlm->target_info, &type2[target_info_offset], target_info_len);
}
}

Expand Down
6 changes: 3 additions & 3 deletions lib/vauth/ntlm_sspi.c
Expand Up @@ -34,6 +34,7 @@
#include "warnless.h"
#include "curl_multibyte.h"
#include "sendf.h"
#include "strdup.h"

/* The last #include files should be: */
#include "curl_memory.h"
Expand Down Expand Up @@ -213,11 +214,10 @@ CURLcode Curl_auth_decode_ntlm_type2_message(struct Curl_easy *data,
}

/* Store the challenge for later use */
ntlm->input_token = malloc(Curl_bufref_len(type2) + 1);
ntlm->input_token = Curl_strndup((const char *)Curl_bufref_ptr(type2),
Curl_bufref_len(type2));
if(!ntlm->input_token)
return CURLE_OUT_OF_MEMORY;
memcpy(ntlm->input_token, Curl_bufref_ptr(type2), Curl_bufref_len(type2));
ntlm->input_token[Curl_bufref_len(type2)] = '\0';
ntlm->input_token_len = Curl_bufref_len(type2);

return CURLE_OK;
Expand Down
11 changes: 3 additions & 8 deletions lib/vssh/wolfssh.c
Expand Up @@ -42,6 +42,7 @@
#include "select.h"
#include "multiif.h"
#include "warnless.h"
#include "strdup.h"

/* The last 3 #include files should be in this order */
#include "curl_printf.h"
Expand Down Expand Up @@ -512,15 +513,9 @@ static CURLcode wssh_statemach_act(struct Curl_easy *data, bool *block)
return CURLE_OK;
}
else if(name && (rc == WS_SUCCESS)) {
sshc->homedir = malloc(name->fSz + 1);
if(!sshc->homedir) {
sshc->homedir = Curl_strndup(name->fName, name->fSz);
if(!sshc->homedir)
sshc->actualcode = CURLE_OUT_OF_MEMORY;
}
else {
memcpy(sshc->homedir, name->fName, name->fSz);
sshc->homedir[name->fSz] = 0;
infof(data, "wolfssh SFTP realpath succeeded");
}
wolfSSH_SFTPNAME_list_free(name);
state(data, SSH_STOP);
return CURLE_OK;
Expand Down
6 changes: 1 addition & 5 deletions lib/vtls/gtls.c
Expand Up @@ -584,13 +584,9 @@ CURLcode gtls_client_init(struct Curl_easy *data,
/* Only add SRP to the cipher list if SRP is requested. Otherwise
* GnuTLS will disable TLS 1.3 support. */
if(config->username) {
size_t len = strlen(prioritylist);

char *prioritysrp = malloc(len + sizeof(GNUTLS_SRP) + 1);
char *prioritysrp = aprintf("%s:" GNUTLS_SRP, prioritylist);
if(!prioritysrp)
return CURLE_OUT_OF_MEMORY;
strcpy(prioritysrp, prioritylist);
strcpy(prioritysrp + len, ":" GNUTLS_SRP);
rc = gnutls_priority_set_direct(gtls->session, prioritysrp, &err);
free(prioritysrp);

Expand Down
11 changes: 5 additions & 6 deletions lib/vtls/mbedtls.c
Expand Up @@ -67,6 +67,7 @@
#include "select.h"
#include "multiif.h"
#include "mbedtls_threadlock.h"
#include "strdup.h"

/* The last 3 #include files should be in this order */
#include "curl_printf.h"
Expand Down Expand Up @@ -367,11 +368,10 @@ mbed_connect_step1(struct Curl_cfilter *cf, struct Curl_easy *data)
/* Unfortunately, mbedtls_x509_crt_parse() requires the data to be null
terminated even when provided the exact length, forcing us to waste
extra memory here. */
unsigned char *newblob = malloc(ca_info_blob->len + 1);
unsigned char *newblob = Curl_strndup(ca_info_blob->data,
ca_info_blob->len);
if(!newblob)
return CURLE_OUT_OF_MEMORY;
memcpy(newblob, ca_info_blob->data, ca_info_blob->len);
newblob[ca_info_blob->len] = 0; /* null terminate */
ret = mbedtls_x509_crt_parse(&backend->cacert, newblob,
ca_info_blob->len + 1);
free(newblob);
Expand Down Expand Up @@ -441,11 +441,10 @@ mbed_connect_step1(struct Curl_cfilter *cf, struct Curl_easy *data)
/* Unfortunately, mbedtls_x509_crt_parse() requires the data to be null
terminated even when provided the exact length, forcing us to waste
extra memory here. */
unsigned char *newblob = malloc(ssl_cert_blob->len + 1);
unsigned char *newblob = Curl_strndup(ssl_cert_blob->data,
ssl_cert_blob->len);
if(!newblob)
return CURLE_OUT_OF_MEMORY;
memcpy(newblob, ssl_cert_blob->data, ssl_cert_blob->len);
newblob[ssl_cert_blob->len] = 0; /* null terminate */
ret = mbedtls_x509_crt_parse(&backend->clicert, newblob,
ssl_cert_blob->len + 1);
free(newblob);
Expand Down
11 changes: 4 additions & 7 deletions lib/vtls/sectransp.c
Expand Up @@ -2369,19 +2369,16 @@ static CURLcode verify_cert(struct Curl_cfilter *cf,
const struct curl_blob *ca_info_blob,
SSLContextRef ctx)
{
int result;
CURLcode result;
unsigned char *certbuf;
size_t buflen;

if(ca_info_blob) {
CURL_TRC_CF(data, cf, "verify_peer, CA from config blob");
certbuf = (unsigned char *)malloc(ca_info_blob->len + 1);
if(!certbuf) {
certbuf = (unsigned char *)Curl_strndup(ca_info_blob->data,
buflen = ca_info_blob->len);
if(!certbuf)
return CURLE_OUT_OF_MEMORY;
}
buflen = ca_info_blob->len;
memcpy(certbuf, ca_info_blob->data, ca_info_blob->len);
certbuf[ca_info_blob->len]='\0';
}
else if(cafile) {
CURL_TRC_CF(data, cf, "verify_peer, CA from file '%s'", cafile);
Expand Down