On Fri, May 4, 2012 at 12:49 PM, Tobias Burnus
<tobias.bur...@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
