Hi Steffan, patch looks good, but I have just one comment below
On 02/11/17 06:03, Steffan Karger wrote:
> If gc == NULL, the data allocated in the alloc_gc_buf() call in
> create_temp_file or the string_mod_const call in gen_path would never
> be free'd.
>
> These functions are currently never called that way, but let's prevent
> future problems.
>
> While touching create_temp_file, also remove the counter variable, which is
> never read, simplify the do-while to a while loop, and truncate the prefix
> (if needed) to preserve the random and extension of the created filename.
>
> Signed-off-by: Steffan Karger <stef...@karger.me>
> ---
> v2:
> - change create_temp_file to avoid using a struct buffer (simpler)
> - add gc != NULL check for gen_path (avoid similar memleak pitfall)
> v3:
> - Check the return value of openvpn_snprintf()
>
> src/openvpn/misc.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 25f38003..67011169 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -723,21 +723,26 @@ test_file(const char *filename)
> const char *
> create_temp_file(const char *directory, const char *prefix, struct gc_arena
> *gc)
> {
> - static unsigned int counter;
> - struct buffer fname = alloc_buf_gc(256, gc);
> int fd;
> const char *retfname = NULL;
> unsigned int attempts = 0;
> + char fname[256] = { 0 };
> + const char *fname_fmt = PACKAGE "_%.*s_%08lx%08lx.tmp";
> + const int max_prefix_len = sizeof(fname) - (sizeof(PACKAGE) + 7 + (2 *
> 8));
>
> - do
> + while (attempts < 6)
> {
> ++attempts;
> - ++counter;
>
> - buf_printf(&fname, PACKAGE "_%s_%08lx%08lx.tmp", prefix,
> - (unsigned long) get_random(), (unsigned long)
> get_random());
> + if (!openvpn_snprintf(fname, sizeof(fname), fname_fmt,
> max_prefix_len,
> + prefix, (unsigned long) get_random(),
> + (unsigned long) get_random()))
> + {
> + msg(M_WARN, "ERROR: temporary filename too long");
> + return NULL;
> + }
>
> - retfname = gen_path(directory, BSTR(&fname), gc);
> + retfname = gen_path(directory, fname, gc);
> if (!retfname)
> {
> msg(M_WARN, "Failed to create temporary filename and path");
> @@ -760,7 +765,6 @@ create_temp_file(const char *directory, const char
> *prefix, struct gc_arena *gc)
> return NULL;
> }
> }
> - while (attempts < 6);
>
> msg(M_WARN, "Failed to create temporary file after %i attempts",
> attempts);
> return NULL;
> @@ -812,6 +816,12 @@ gen_path(const char *directory, const char *filename,
> struct gc_arena *gc)
> #else
> const int CC_PATH_RESERVED = CC_SLASH;
> #endif
> +
> + if (!gc)
> + {
> + return NULL; /* Would leak memory otherwise */
As far as I understood the ratio behind this change, we are basically
saying that passing gc == NULL is a real programming error and not a
runtime problem. If so, wouldn't it be better to assert(gc) directly?
I am saying so because we should make this failure louder, so that it
can be recognized in an early test.
Or can we have cases when this function is called with NULL because of a
temporary failure (my understanding is that this should not be possible)?
Cheers,
> + }
> +
> const char *safe_filename = string_mod_const(filename, CC_PRINT,
> CC_PATH_RESERVED, '_', gc);
>
> if (safe_filename
>
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
