[GRASS-dev] G.proj requirements?

[Glynn Clements]

Paul Kelly wrote:

> >> (b) If the call to G_gisinit() is simply commented out, the next problem
> >> is G_tempfile() [deep within GRASS gproj library] which tries to create a
> >> tempfile in the current mapset as far as I remember - is this a good/bad
> >> idea? Certainly bad if you don't have a current mapset.
> >
> > G_tempfile() uses the current mapset so that "draft" output maps can
> > be moved (with either link()+remove() or rename()) into their final
> > location upon closure. This requires them to reside on the same
> > filesystem (partition) as the mapset directory (i.e. you can't use
> > /tmp or $TMPDIR).
> >
> > I suggest:
> >
> > 1. Rename the existing G_tempfile() function.
> >
> > 2. Add a new G_tempfile() which uses the /tmp/grass6-<user>-<pid>
> > directory (or possibly $TMPDIR) instead.
> 
> Right. It was mentioned before that tempnam() could be used - although I 
> see on Linux that warns that it shouldn't---but I don't understand why. 

Race conditions.

Because tempnam() doesn't create the file, it's possible for another
user's process to create a file (or, more significantly, a link to an
existing file) with that name before your process does. This is
considered a potential security vulnerability, as an attacker can
trick a user into creating or modifying files which the attacker
wouldn't have access to.

mkstemp() and tmpfile() both attempt to create the file, and only
return once they have successfully created a new file.

> The /tmp/grass6-<user>-<pid> location is tied in tightly to the concurrent 
> session stuff in Init.sh isn't it---I'd rather not get into complications 
> of relying on this if possible because it reduces the simplicity of only 
> having to set a few environment variables (GISBASE and GISRC for GRASS to 
> run). E.g. in the experimental Windows version of Init.sh (init.bat) I 
> have not even bothered implementing concurrent sessions yet.
> 
> tempnam() uses $TMPDIR if set, which means it seems to work on Windows, or 
> a specified directory as the second choice. This seems to work OK:
> 
> char *G_tempfile_in_mapset(void)
> {
>      return G__tempfile(getpid());
> }
> 
> char *G_tempfile(void)
> {
>      return G_convert_dirseps_to_host( tempnam("/tmp", "grass") );
> }
> 
> 
> but I can see deleting of temporary files after a session being a problem. 
> The prefix for the temp files (in this case "grass") can be at most 5 
> characters with tempnam() and I don't think it would be desirable to go 
> looking in the temporary directory and deleting any files that started 
> with that at the end of a session. Using the per-session directory would 
> of course get round that problem, so maybe it is worth a second thought...

Apart from anything else, any security issues can be eliminated by
ensuring that the per-session directory is mode 0700.

> The other option is to keep a lookout after changing it for badly-behaved 
> modules that don't delete their temp files. Also I don't think it's 
> unreasonable for temp files to be left undeleted if a module crashes? 
> Might be useful for debugging even.

Apart from anything else, the effort involved in cleaning up upon
G_fatal_error() is too much. At least, it's not practical to do it
module-by-module. You need a global mechanism, e.g. ensuring temp
files always contain the PID of their creator, so that G_fatal_error()
can just delete every temp file containing the current PID.

That still doesn't allow for crashes, but safely recovering from
SIGSEGV is next to impossible. If you suspect that application memory
has been corrupted, you certainly don't want to be calling remove()
etc on filenames which are derived from the contents of application
memory.

[Even constructing a filename from a combination of string literals
(which are read-only) and kernel data (e.g. getpid()) isn't as safe as
it looks, as references to functions in shared libraries may have been
corrupted, meaning that a getpid() call might invoke something other
than the actual "getpid" function.]

> > 3. Change close_new() (lib/gis/closecell.c, used by G_close_cell() and
> > G_unopen_cell()) to use the existing (renamed) function. Ditto for
> > anything else which creates temporary files with the intention of
> > moving them (if there is anything else).
> 
> opencell.c seemed to be the file that actually required changing?

Yeah; opencell.c creates the file in that way so that closecell.c
works.

> But only two places to change it and very easy.

That's assuming that other subsystems don't do the same thing.

Ideally, a lot more code should be doing this. At present, it's only
the actual cell/fcell file which is replaced atomically; other files
(e.g. cellhd) are modified in-place.

If we switched the layout from <element>/<map_name> to
<map_name>/<element> (i.e. one directory for each map), the obvious
approach would be to rename the entire directory rather than
individual files.

-- 
Glynn Clements <glynn at gclements.plus.com>