Re: lynx-dev Options, V.Links, random (was: patch 6 - UI Pages)

[Klaus Weide]

On Thu, 16 Dec 1999, T.E.Dickey wrote:

> > >   + is the code that opens temp-filenames working properly when it finds 
> > >     a collision. 
> >  
> > I think the last one is the critical one.  If the answer is YES, the rest 
> > hardly matters. 
> >  
> >   Klaus 

In the above, I was thinking - and I thought you were thinking - about
the more system-dependent stuff "below" LYOpenTemp, esp. whether the
permission-checking/directory-traversing stuff in IsOurFile is always
right and whether OpenHiddenFile always reacts appropriately.

You aren't changing anything about *that* below - so I'll continue to
assume that there are no known problems with it, and that it works fine
(given a "sane" environment, at least).

[ Which, according to what I wrote above, means "the rest hardly matters" :)  -
in particular, it shouldn't matter whether filenames are obvious or guessable. ]

Anyway, to the below -

If you insist on keeping the random stuff, I hope you'll let me easily
turn it off.  Introducing USE_RAND_TEMPNAME seems to be a step in that
direction.  That's good...

Looking through your changes, at first I thought, yes, that should
work.  It's unnecessarily limiting (10000 files only :) ); inelegant
(all those list traversals - and I'd like to see the slowdown as
number-of-files approaches 10000, must be like a rocket approaching
speed-of-light :) ); and error prone (there would be bad effects if,
by some error elsewhere, temp files are tracked incorrectly, so they
are kept in the ly_temp list when they shouldn't).

You add some unpredictability to the random seed.  Well it's enough
so that *I* cannot guess it.  I think someone still will be able to.
Well okay; you don't claim that it is perfect.


However, on looking closer, the change doesn't really achieve what it
should.  The assumption you make is that a filename only needs to be
tracked (for avoiding name reuse) for as long as Lynx already *is*
tracking it (for the purpose of file cleanup on exit, mostly).

That is not always a safe assumption.  

Consider the following situation:

-  REUSE_TEMPFILES:FALSE (the default)
-  Option Menu style being used is form-based
-  User has invoked 'O', then (neither submitted changes nor done a PREV_DOC
   but) gone forward to another page (e.g., by following the "HELP" link).
   User invokes 'O' again.

The history stack now looks like this:

You selected:
 53. [2]Options Menu
     file://localhost/tmp/L12845-320TMP.html
 52. [3]Options Screen Help
     file://localhost/usr/local/lib/lynx_help/keystrokes/option_help.html.gz
 51. [4]Options Menu
     file://localhost/tmp/L12845-319TMP.html

The file in position 51, a previous instance of the Options Menu
contents, doesn't exist any more.  It isn't being tracked in the
ly_temp list either.  Nevertheless, the URL still exists in the
history stack, and the rendered document still exists in the Lynx cache
of rendered documents (assuming here that  the '-cache' number is >= 3).  

(Assume some other actions here, creating other temp files - doesn't really
matter.  Just return to the situation shown above.)

At this point, you can go back to #51: PREV_DOC twice.
There are two cases that are normal:
a) #51 is still in the cache of rendered docs.  So it gets displayed.
   When it is shown, RELOAD will result in an error.  It should - the
   file doesn't exist any more.
b) #51 isn't cached any more.  "Returning" to it results in an error
   message (and skipping it on the history stack).
[ If you think that's *not* what should "normally" happen - well, see if
  it does.  You could use REUSE_TEMPFILES to change behavior, but that
  would then be a different case. ]
The possibility of filename reuse opens up a third case, one that is
clearly wrong:
c) "Returning" to #51 results in loading and display of the wrong contents.

With your code below, (c) is still possible.  The filename may have been
reused, and the file may exist again with unexpected contents.


> I think this addresses that point (a little longer than needed since I swept
> up some repeated code into functions):
> 
> --- LYUtils.c.orig    Wed Dec 15 06:25:06 1999
> +++ LYUtils.c Thu Dec 16 06:39:56 1999
> @@ -3732,26 +3773,21 @@
>      /*
>       * Prefer a random value rather than a counter.
>       */
> -#if defined(HAVE_RAND) && defined(HAVE_SRAND) && defined(RAND_MAX)
> +#ifdef USE_RAND_TEMPNAME
> +    do {
> +     int uninit;     /* ( intentionally uninitialized ;-) */

I suspect a compiler might be allowed to optimize this away, somehow.
Suggestion: add 'volatile'.

Well, "intentionally uninitialized" could be anything.  It could for
example always be 0, on a given system.  There's no guarantee about
it being 'random' at all.  It may happen to preserve arbitrary previous
memory content on most (Unix-like, at least systems).  Don't rely on
it.  The potential spoofer may know better than we.

>      if (counter == 0)
> -     srand(time((time_t *)0));
> -    counter = ( 10000.0 * rand() ) / RAND_MAX;
> +     srand(time((time_t *)0) + uninit);
> +    /* We don't really need all of the bits from rand().  The high-order bits
> +     * are the more-random portion in any case, but limiting the width of the
> +     * generated name is done partly to avoid problems on systems that may 
> not
> +     * support long filenames.
> +     */
> +    counter = ( (float)MAX_TEMPNAME * rand() ) / RAND_MAX + 1;

YM

> +    counter = ( (float)MAX_TEMPNAME * rand() ) / (RAND_MAX + 1.0);

or

> +    counter = ( (float)MAX_TEMPNAME * rand() ) / (RAND_MAX + 1.0) + 1;

?

>  #else
>      counter++;
>  #endif

> @@ -3787,6 +3823,10 @@
>       sprintf(result, "%.*s", LY_MAXPATH-1, leaf);
>       code = FALSE;
>      }
> +#ifdef USE_RAND_TEMPNAME
> +    /* If we really have 10000 files open, there's no point in returning... 
> */
> +    } while (FindTempfileByName(result) != 0);

This is the important point.  You are checking ly_temp, but not whether
any document points to that file, nor whether LYIsUIPage is true, nor
whether any other kinds of pointers still exist to that filename or URL
that might result in another attemt to access it, (nor whether the file
really exists).  No, I don't suggest you add any such checks - I still
suggest forget about the rand() in filenames...

  Klaus