Re: mktemp write failure

[Jim Meyering]

Eric Blake wrote:

> Jim Meyering <jim <at> meyering.net> writes:
>
>> I'm pretty sure I've already audited each of those, and assured myself
>> that they were not vulnerable to this sort of trouble -- but that was back
>> when we added other *safer functions.  Thus, if they currently solve no
>> problem, I'm a little reluctant to impose the wrappers.  On the other,
>> I haven't measured the cost of going through the wrapper, and we've just
>> seen how the lack of the wrapper can cause an admittedly-obscure failure.
>>
>> It is probably worth making this change regardless, just because
>> the issue is so subtle.
>>
>> Have you considered automating (make syntax-check style) the task of
>> ensuring that stdio--.h is included at least whenever freopen is used?
>
> Like this?  Rebased against today's changes.  As shown below in patch 3/3, the

Yes.  Thanks.

> patch requires stdio--.h only for freopen.  But I also tried this regex in
> sc_require_stdio_safer:
>
>         files=$$(grep -l '\b\(f\|fre\|p\)open \?(' $$($(VC_LIST_EXCEPT) \
>
> and found the following culprits (I think they were all due to fopen).
>
> src/base64.c
> src/cksum.c
> src/cut.c
> src/date.c
> src/expand.c
> src/fmt.c
> src/fold.c
> src/nl.c
> src/od.c
> src/paste.c
> src/pinky.c
> src/sum.c
> src/unexpand.c
> src/uptime.c
> src/wc.c
> maint.mk: the above files should use "stdio--.h"

Let's wrap only freopen for now.
I had reservations about the wrapping being unnecessary
for most freopen uses...

If someone can spot a problem with an fopen or popen use,
that would lend weight to the "just wrap them all" approach.

> I checked out base64; it looks like it could possibly benefit from using 
> freopen
> (,stdin) instead of fopen, but I couldn't come up with any scenario where
> starting life with a closed fd would cause problems (with >&-, input_fh ends 
> up
> on descriptor 1, but since fd 1 is O_RDONLY in that case, attempts to use
> stdout still gave the expected failure).  But that doesn't mean there aren't
> any issues, since I didn't try using things like /proc/self/fd/1 as the file
> name.  Likewise, I did not audit the rest of the above list to see if
> fopen_safer would make an observable difference.
>
> On the other hand, we are not calling fopen frequently, so it is not on the 
> hot
> path, and the time spent in an extra wrapper for safety is probably
> insignificant.  So, should I respin this patch to use the regex that catches
> fopen, and add "stdio--.h" to even more files?  Or maybe even squash patch 2
> and 3 into a single one before pushing?

Squashing sounds fine.

One potential problem:
The removal of the
  #include <stdio.h>
that precedes e.g., #include <assert.h>

I recall (possibly on systems too old to be relevant) that including
assert without first including stdio.h would cause compilation failure.
Probably not worth worrying about.

> diff --git a/src/du.c b/src/du.c
> index c33bbb7..93a33cf 100644
> --- a/src/du.c
> +++ b/src/du.c
> @@ -24,7 +24,6 @@
>     Rewritten to use nftw, then to use fts by Jim Meyering.  */
>
>  #include <config.h>
> -#include <stdio.h>
>  #include <getopt.h>
>  #include <sys/types.h>
>  #include <assert.h>
> @@ -40,6 +39,7 @@
>  #include "quotearg.h"
>  #include "same.h"
>  #include "stat-time.h"
> +#include "stdio--.h"
>  #include "xfts.h"
>  #include "xstrtol.h"
>
> diff --git a/src/tsort.c b/src/tsort.c
> index 09067f2..cc6807a 100644
> --- a/src/tsort.c
> +++ b/src/tsort.c
> @@ -22,7 +22,6 @@
>
>  #include <config.h>
>
> -#include <stdio.h>
>  #include <assert.h>
>  #include <getopt.h>
>  #include <sys/types.h>
> @@ -32,6 +31,7 @@
>  #include "error.h"
>  #include "quote.h"
>  #include "readtokens.h"
> +#include "stdio--.h"
>
>  /* The official name of this program (e.g., no `g' prefix).  */
>  #define PROGRAM_NAME "tsort"