Re: ztempfile

On Fri, Aug 15, 2003 at 04:17:45AM +0400, Igor V. Melichev wrote:
> Ray, Raph,
> 
> After thinking more about the subject,
> now I see that the problem actually is in another place.
> 
> The semantics assumed for gp_gettmpdir is to retrieve
> a path to a directory for temporary files. The result
> is platform dependent, because it starts with a drive latter on Windows,
> etc.
> Note that here I say about the result rather than implementation.

Of course the value depends on the platform. That's why it is a gp_
function.

> The problem happens when the platform-dependent result is tried to
> obtain with a platform-independent implementation. Really on Unix we want
> the path to be /temp when the environment variable isn't set.
> But due to an unclear reason we don't want this on Windows
> (I mean C:/temp ).
> 
> Thus the function gp_gettmpdir appears to have a wrong implementation
> for Unix. I believe that the main big is here.

gp_gettmpdir() is perhaps poorly named - it merely retrieves a
user-specified temp directory from the environment variables. From the
name, one could wrongly guess that it actually determines the
directory in all cases. In any case, I think my comments in the
banner for gp_open_scratch_file() make it sufficiently clear that
the responsibility for choosing the temp directory lies squarely with
that function.

> (Another bug is that on Windows gp_gettmpdir returns something else
>  when TMPDIR is set. Such behavior is not usual for Windows applications.
>  This bug may be considered as a minor one or an important one depending on
>  personal preferences.
> )

If it is desirable to prefer the result of GetTempDir() over the TMPDIR
environment variable, that can be implemented simply by removing the
call to gp_gettmpdir() from the gp_mswin.c implementation of
gp_open_scrath_file().

> Then there are 2 ways for fixing gp_gettmpdir :
> 
> 1. Make gp_gettmpdir to be platform-dependent (i.e. move it to
> platform-dependent modules, and implement specifically.
> 
> 2. Define a generic semantics for all platforms :
> 
>    - get the name list of environment variables (a constant, platform
> dependent)
>    - take the value of the first defined variable through the list.
>    - if there is no value or it is empty, use a default absolute path
>      (a constant, platform dependent, or a system call).
> 
> The way (2) appears some complicated, but I prefer it because
> it defines a generic semantics.
> In any case, gp_gettmpdir must never return an empty path.

I still believe that the best way is to have gp_open_scratch_file()
be responsible for all this logic, as my proposal implements.

> Besides all this, none of us mentioned another important case :
> the environment variable has a non-empty value, which does
> not start with root, cwd or parent.
> Note : it is not the Raph's constraint for the .tempfile argument.
> Both the current code and the Raph's
> proposal have a strange behavior in this case in the point of security.
> I suggest to improve the semantics of gp_gettmpdir once again :
> it should ever retrieve a path starting with root, cwd, or parent.
> Specifically, if the environment variable specifies an "abnormal" value,
> the value is prefixed by the default (platform-dependent) path,
> which is /tmp on Unix, C:/temp on Windows and so on.

We did talk about this case on the phone. I think it's reasonable
to expect GS to create tempfiles in the location specified by the user,
whether it's relative or absolute. Of course, the latter will be more
robust, but I don't think it's our job to nanny.

> As to gp_open_scratch_file, it should not attach something before
> the specified 'prefix'. ztempfile computed a path and checked the
> permissions,
> so gp_open_scratch_file must open a file exactly on this path.
> With the current permission logics the path can be either absolute
> or start from cwd or parent. The code of gp_open_scratch_file
> to be changed with
> 
> If this approach is taken, there is no need to change ztempfile.
> I believe that the current code in CVS performs its job perfectly.

I disagree strongly. One of the most important cases in which the
current code fails is that it checks permission in the current
directory, even for "simple" prefixes. This is wrong.

Raph