Hi, On Wed, Aug 26, 2015 at 12:29:13AM +0200, Steffan Karger wrote: > As reported by Bill Parker in trac #600, strdup() return values are not > always correctly checked for failed allocations. This patch adds missing > checks. > > Note that in misc.c and options.c, the check is after the dirname() or > basename() call, because these can deal with NULL params and we need to > keep MSVC happy with its ancient no-declaration-after-statement policy.
Even though Arne has already ACKed it, I'm not merging it - I started
merging it today, but the more I stare at the code, the more I'm convinced
that we should just get rid of strdup() instead and/or the code is not
needed.
Why...? Let me comment...
> diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
> index b7fc11e..62f30be 100644
> --- a/src/openvpn/cryptoapi.c
> +++ b/src/openvpn/cryptoapi.c
> @@ -127,6 +127,7 @@ static char *ms_error_text(DWORD ms_err)
> break;
> }
> }
> + check_malloc_return(rv);
> return rv;
> }
This is not needed, as the code does
rv = strdup(lpMsgBuf);
LocalFree(lpMsgBuf);
/* trim to the left */
if (rv)
for (p = rv + strlen(rv) - 1; p >= rv; p--) {
if (isspace(*p))
*p = '\0';
else
break;
}
}
return rv;
... so if the strdup() fails, the function falls into "return NULL",
which is the same return value as for "if (!lpMsgBuf)", so I assume
the caller expects this - with the extra check, a failure in
FormatMessage() would be flagged as memory allocation failure (because
rv is set to NULL at start), which it isn't.
So, just not needed.
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index b7c153b..0809cc7 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -817,6 +817,7 @@ init_options_dev (struct options *options)
> {
> if (!options->dev && options->dev_node) {
> char *dev_node = strdup(options->dev_node); /* POSIX basename()
> implementaions may modify its arguments */
> + check_malloc_return(dev_node);
> options->dev = basename (dev_node);
> }
> }
I'd need to look into this in more detail, but I think we never modify
options->dev_node afterward - so a simple strrchr() to find the "/" and
point options->dev there might be sufficient... but if it actually needs
to be a true copy, we should just use
char *dev_node = string_alloc(options->dev_node, NULL)
instead - string_alloc() is OpenVPN's strdup(), and "NULL" tells it
"do not use gc, use calloc" (side note: someone seriously overengineered
the "we really want clean memory!" bit here - allocating <n> bytes and
immediately overwriting with a n-byte memcpy() should be good enough,
no?) - *and* string_alloc() actually does...
if (gc) {
...
} else {
ret = calloc(1, n);
check_malloc_return(ret);
}
memcpy (ret, str, n);
... so the check_malloc_return() is built-in.
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index a5bad0d..b543b65 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -1608,6 +1608,7 @@ argv_extract_cmd_name (const char *path)
> {
> char *path_cp = strdup(path); /* POSIX basename() implementaions may
> modify its arguments */
> const char *bn = basename (path_cp);
> + check_malloc_return(path_cp);
> if (bn)
> {
> char *ret = string_alloc (bn, NULL);
This is totally weird code, using strdup() first, and then string_alloc()
later, inside an "if (bn)" check on a variable that will never be NULL
anyway, since basename(NULL) is documented to return "." - and it very
much looks like two different persons wrote this part :-)
Dunno, this really smells like refactor the function to use strrchr()
on the original path, and then use string_alloc($ptr, NULL) to make
a copy of that... throw out the if(bn) block, and be done... or just
change the strdup() to string_alloc() as well.
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 2784580..dba7c9d 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -2588,6 +2588,7 @@ check_file_access(const int type, const char *file,
> const int mode, const char *
> {
> char *fullpath = strdup(file); /* POSIX dirname() implementaion may
> modify its arguments */
> char *dirpath = dirname(fullpath);
> + check_malloc_return(fullpath);
>
> if (platform_access (dirpath, mode|X_OK) != 0)
> errcode = errno;
string_alloc() (pity we do not have a gc around here :-) )
> diff --git a/src/openvpn/ssl_polarssl.c b/src/openvpn/ssl_polarssl.c
> index 3fc811e..673dbbe 100644
> --- a/src/openvpn/ssl_polarssl.c
> +++ b/src/openvpn/ssl_polarssl.c
> @@ -198,6 +198,7 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const
> char *ciphers)
> /* Parse allowed ciphers, getting IDs */
> i = 0;
> tmp_ciphers_orig = tmp_ciphers = strdup(ciphers);
> + check_malloc_return(tmp_ciphers);
>
> token = strtok (tmp_ciphers, ":");
> while(token)
I could mention string_alloc() again, but I'm sure interested readers
already recognize the pattern :-))
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025 g...@net.informatik.tu-muenchen.de
pgp4ftFfP_EmE.pgp
Description: PGP signature
