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

[T.E.Dickey]

> 
> 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 

right (I didn't plan to redesign the temp-file logic, but mistook an aspect
of logic in LYOpenTemp for part of what I needed for the random-sequence)

> 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... 

it was a reasonable thing considering that I'd the same ifdef's in two places.
  
> 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). 

crude yes -
otoh, it's still much less time than the actual file-system checks up above
in LYOpenTemp.
  
> 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: 

but my change limits rather than expands the set of filenames available for
opening.

> c) "Returning" to #51 results in loading and display of the wrong contents. 

but is this related to the random-sequence?  (I did not see anyplace where
we use the actual sequence number anymore - it's done by filename).  If
it's not related to the random-sequence, it would still be a bug which we
should fix (but a different topic).
  
> 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'. 

if the compiler supports it - ok - though iirc, some versions of gcc were
reported to do incorrect things with volatile.  Since the variable is "used",
I don't think it would be optimized away (but it'll generate an annoying
warning ;-)
  
> Well, "intentionally uninitialized" could be anything.  It could for 

(the comment was to keep someone from fixing the warning)

> 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 

the latter, I'd expect (whatever's on the stack is the usual behavior).

> > +#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... 

but I _am_ checking if Lynx has opened the file as a temporary file, and if
Lynx has not explicitly discarded it.
  
>   Klaus 
>  


-- 
Thomas E. Dickey
address@hidden
http://www.clark.net/pub/dickey