Re: download tmpnam patch

"Navin P" <[email protected]> writes:

> ! 	unsigned char *nm,tm[22],*km,*pm;

How did you get this number 22?

#   define ULLONG_MAX	18446744073709551615ULL

Here, ULLONG_MAX has 20 digits and '\0' is one more character, so
unsigned char tm[21] would have sufficed.  But long long might
have more than 64 bits, and so might RAND_MAX or time_t.

> ! 	unsigned long long t,k;

For portability, ELinks does not use long long unconditionally.
If you want to use long long, you should check whether
HAVE_LONG_LONG is defined, and do something else if not.
See the #define longlong in src/osdep/types.h.

> ! 	srand(time(NULL));
> ! 	t=rand()+ time(NULL) + 1LL;
> ! 	sprintf(tm,"%llu",t);

I do not think calling srand repeatedly is appropriate.

> ! 	/* This function already exists.We are using
> ! 	 * some rand() as prefix in get_tempdir_filename
> ! 	 * .This should fix the race condition even if
> ! 	 * the file exists.The rand() is not necessarily
> ! 	 * required but helps.
> ! 	 * --navin */

rand() cannot fix the race condition, it can only make it more
difficult to reproduce.  Anyway if it were good to have a random
element in names of temporary files then I think that code should
be in get_unique_name rather than here, so that all callers could
benefit from it.

> ! 	nm=get_unique_name(pm);
> 
>   	if (!init_string(&name)) {
> ! 		mem_free(nm);
> ! 		mem_free(pm);

get_unique_name apparently returns the original pointer if the
name is already unique.  Your code frees the same memory twice
in that case.

>   	extension = get_extension_from_uri(uri);
>   	if (extension) {

First get_unique_name (or tempnam) tries different names until it
finds one for which there is no file yet, and then get_temp_name
appends an extension, so the result of the check is no longer
valid.  Thus get_temp_name doesn't even notice clashes with files
that exist before it is called.  Is that why you needed rand()?

If get_unique_name is to be used then I think it should take the
extension as another parameter, so that it can append it before
checking whether the file already exists.  This might require
changing lookup_unique_name, too.

> - 		/* FIXME: get_temp_name() calls tempnam(). --Zas */
>   		file = get_temp_name(type_query->uri);

If this FIXME is removed, I think there should be a comment for
each variable and data member where the string is stored, warning
that another user may have created a file or symlink with that
name after the name was chosen.

_______________________________________________
elinks-dev mailing list
[email protected]
http://linuxfromscratch.org/mailman/listinfo/elinks-dev

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)

iD8DBQFFkgbYHm9IGt60eMgRArgEAJ9KTDjIkaD6lju0ufYKxIrNnbHBKgCghYJI
TpnL0SvOy6PxoqB84L4nlhU=
=+O/Z
-----END PGP SIGNATURE-----