Re: [Patch, libfortran] Fix handling of temporary files

[Janne Blomqvist <blomqvist dot janne at gmail dot com>]

On Fri, May 4, 2012 at 12:49 PM, Tobias Burnus
<tobias.burnus@physik.fu-berlin.de> wrote:
> Dear Janne,
>
> sorry for the really slow review.
>
> Janne Blomqvist wrote:
>> the attached patch implements some fixes for handling STATUS="SCRATCH" files.
> ...
>> Currently, we check [...] but not the POSIX and GNU standard TMPDIR [...]
>> If the program is privileged, we shouldn't trust path style environment variables.
>
> Good chatch!
>
>
> +On the MingW target, the directory returned by the @code{GetTempPath}
> +#define P_tmpdir _P_tmpdir Â/* MingW */
>
> It's spelt MinGW ("Minimalist GNU for Windows").

Will fix.

> Â* GFORTRAN_STDERR_UNIT:: Unit number for standard error
> -* GFORTRAN_TMPDIR:: Directory for scratch files
> +* TMPDIR:: Directory for scratch files
> Â* GFORTRAN_UNBUFFERED_ALL:: Do not buffer I/O for all units.
>
> I would re-order the list and place TMPDIR either at the top or at
> the bottom. Given that it is a more useful environment variable,
> placing it at the top might make sense.
> (You should then also move the actual text.)

Good point. Will do.

> + /* Check for special case that tempdir contains slash
> Â Â Âor backslash at end. Â*/
>
> (second line looks wrongly indented.)
>
> Additionally, I would add a couple of articles:
> Â"Check for the special case that tempdir contains a slash
> Â or backslash at the end"
> or, better,
> Â"Check for the special case that tempdir ends with a slash
> Â or backslash"

That sentence was already present before the patch, but I'll fix that as well.

> +tempfile_open (const char * tempdir, char ** fname)
>
> GNU style: No spaces after "*".

Hmm, I thought that GNU style was that there should be spaces both
before and after the "*". But now that I look at
https://www.gnu.org/prep/standards/standards.html#Formatting it seems
that the "*" should indeed be grouped together with the variable name.
 Which is nice, since I never liked what I thought the GNU style was!
;-)  I just wonder where I got that idea from..

> +}
> +
> +/* tempfile()-- Generate a temporary filename for a scratch file and
>
> Second empty line missing.

Right, will fix.

> + Âif (!tempdir)
> + Â Âreturn -1;
> ...
> + Âsize_t tempdirlen = strlen (tempdir);
>
> I am surprised that it compiles. I thought GCC is compiled with C89
> settings such that variables have to be declared before the the
> expression statements start. Did you check whether it compiles with
> any warnings?

libgfortran is compiled with -std=gnu99, so this is Ok. We use C99
features quite a lot in libgfortran, FWIW.

IIRC there were no warnings.

> Otherwise, the patch looks fine.
>
> Thanks for the patch!

Thanks for the review!

> Tobias
>
> PS: Side note, I find the document produced by the ISO/IEC Project 22.24772:
> "Guidance for Avoiding Vulnerabilities through Language Selection"
> useful; while it is rather lengthy and not that readably written, it still
> contains some helpful bits. However, it does not really address the
> environment variable issue which you mentioned (ignoring 6.47.1 and
> 3.1.2.12 which are only loosely related).
> Cf. http://gcc.gnu.org/wiki/GFortranStandards#ISO.2BAC8-IEC_Project_22.24772:_Guidance_for_Avoiding_Vulnerabilities_through_Language_Selection_and_Use


Some info wrt secure tmp file handling and env variables:

http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html#TEMPORARY-FILES
 (see code example, which does more or less what the secure_getenv
fallback in the patch does. Actually, that entire HOWTO is interesting
in general)

https://www.gnu.org/software/libc/manual/html_node/Temporary-Files.html

-- 
Janne Blomqvist