tmpfile leaks

tmpfile leaks

[Andy Wingo]

Coverity identified the following error:

> Error: RESOURCE_LEAK:
> /builddir/build/BUILD/guile-2.0.2/libguile/posix.c:1342: alloc_fn: Calling allocation function "tmpfile".
> /builddir/build/BUILD/guile-2.0.2/libguile/posix.c:1342: var_assign: Assigning: "rv" =  storage returned from "tmpfile()".
> /builddir/build/BUILD/guile-2.0.2/libguile/posix.c:1344: noescape: Variable "rv" is not freed or pointed-to in function "fileno".
> /builddir/build/BUILD/guile-2.0.2/libguile/posix.c:1344: leaked_storage: Variable "rv" going out of scope leaks the storage it points to.

And indeed:

  SCM_DEFINE (scm_tmpfile, "tmpfile", 0, 0, 0,
              (void),
              "Return an input/output port to a unique temporary file\n"
              "named using the path prefix @code{P_tmpdir} defined in\n"
              "@file{stdio.h}.\n"
              "The file is automatically deleted when the port is closed\n"
              "or the program terminates.")
  #define FUNC_NAME s_scm_tmpfile
  {
    FILE *rv;

    if (! (rv = tmpfile ()))
      SCM_SYSERROR;
    return scm_fdes_to_port (fileno (rv), "w+", SCM_BOOL_F);
  }
  #undef FUNC_NAME

What should we do here?

Andy
-- 
http://wingolog.org/

Re: tmpfile leaks

[Chris K. Jester-Young]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Fri, Jul 29, 2011 at 09:07:43AM +0200, Andy Wingo wrote:
> What should we do here?

There's two ways I can think of:

1. Basically reimplement the functionality of tmpfile, using mkstemp
   and unlink, passing the fd to scm_fdes_to_port as before. This does
   require reimplementing the TMPDIR lookup stuff (since that doesn't
   seem to be exposed by libc).

2. Use tmpfile as before, dup the file descriptor, and close the stdio
   stream unconditionally. This is wasteful of a file stream in that
   one gets created only to be immediately destroyed, but it's more
   faithful to the system-provided tmpfile, quirks and all.

In the end, I chose approach 2 (more daring coders may want to have a
go at 1 :-)). Attached is a diff.

Cheers,
Chris.

[-- Attachment #2: posix.diff --]
[-- Type: text/x-diff, Size: 516 bytes --]

diff --git a/libguile/posix.c b/libguile/posix.c
index 2923dc6..8dea378 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1338,10 +1338,15 @@ SCM_DEFINE (scm_tmpfile, "tmpfile", 0, 0, 0,
 #define FUNC_NAME s_scm_tmpfile
 {
   FILE *rv;
+  int fd;
 
   if (! (rv = tmpfile ()))
     SCM_SYSERROR;
-  return scm_fdes_to_port (fileno (rv), "w+", SCM_BOOL_F);
+  fd = dup (fileno (rv));
+  fclose (rv);
+  if (fd == -1)
+    SCM_SYSERROR;
+  return scm_fdes_to_port (fd, "w+", SCM_BOOL_F);
 }
 #undef FUNC_NAME
 

Re: tmpfile leaks

[Ken Raeburn]

On Jul 29, 2011, at 12:04, Chris K. Jester-Young wrote:
> On Fri, Jul 29, 2011 at 09:07:43AM +0200, Andy Wingo wrote:
>> What should we do here?
> 
> There's two ways I can think of:
> 
> 1. Basically reimplement the functionality of tmpfile, using mkstemp
>   and unlink, passing the fd to scm_fdes_to_port as before. This does
>   require reimplementing the TMPDIR lookup stuff (since that doesn't
>   seem to be exposed by libc).
> 
> 2. Use tmpfile as before, dup the file descriptor, and close the stdio
>   stream unconditionally. This is wasteful of a file stream in that
>   one gets created only to be immediately destroyed, but it's more
>   faithful to the system-provided tmpfile, quirks and all.

Better check whether these will work on Windows, where file deletion interacts differently than on UNIX.

Ken

Re: tmpfile leaks

[Andy Wingo]

On Sat 30 Jul 2011 07:02, Ken Raeburn <raeburn@raeburn.org> writes:

> On Jul 29, 2011, at 12:04, Chris K. Jester-Young wrote:
>> 2. Use tmpfile as before, dup the file descriptor, and close the stdio
>>   stream unconditionally. This is wasteful of a file stream in that
>>   one gets created only to be immediately destroyed, but it's more
>>   faithful to the system-provided tmpfile, quirks and all.

I have applied something like this; thanks.

> Better check whether these will work on Windows, where file deletion
> interacts differently than on UNIX.

It won't work there.  We'd need to set up some sort of deferred thing to
delete the file.  Perhaps better to not provide tmpfile on mingw.

Andy
-- 
http://wingolog.org/