[prev in list] [next in list] [prev in thread] [next in thread] 

List:       clamav-devel
Subject:    Re: [Clamav-devel] Fixes for: ERROR: ScanStream: Can'tcreatetemporary
From:       "Mark Pizzolato" <clamav-devel () subscriptions ! pizzolato ! net>
Date:       2004-12-02 15:10:35
Message-ID: 004e01c4d881$15fc17c0$173ca8c0 () AlohaSunset ! com
[Download RAW message or body]

On Thursday, December 02, 2004 2:43 AM, Tomasz Kojm wrote:
> On Wed, 1 Dec 2004 18:32:33 -0800
> "Mark Pizzolato" <clamav-devel@subscriptions.pizzolato.net> wrote:
>
> > > Every normal POSIX compliant platform provides reentrant and thread
> > > safe tmpfile().
> >
> > Solaris man pages for tmpnam(), and tempnam() and tmpfile() suggest
> > that tmpfile() uses tmpnam() and that tmpnam() is not threadsafe.
>
> No, it doesn't. It just says that "The tmpnam() function is unsafe  in
> multithreaded  applications. The  tempnam()  function  is  safe in
> multithreaded applications and should be used instead." No word about
> tmpfile().
>
> Also `man -s5 attributes` doesn't list tmpfile() as a MT-unsafe
> function.

That is true, however the tmpfile() page specifically mentions tmpnam() (not 
tempnam())AND unlike other functions which are MT-Level "Safe", makes no 
mention of MT-Level-ness.  Given that tmpnam() is explicitly unsafe and the 
direct dependent reference to tmpnam() from tmpfile() and  Lacking access to 
the source, a reasonable conclusion is that tmpfile() like tmpnam() is MT 
unsafe.

> > exists a real likelyhood that collisions will occur on systems which
> > have relatively low resolution updates to the microsecond return from
> > gettimeofday().
>
> To protect against potential collisions it uses a last result of
> cli_md5buff as a salt.

Ahh.  Now I see.  Given this approach, what is being protected by the mutex 
in cli_gentemp() ?

By the way, it would seem that cli_rndnum()  should return a result modulo 
(max+1) instead of max if max truly represents tht maximum value to be 
returned.  It seems from the only usage of cli_rndnum() that the caller is 
presuming maximum value.

OK, so there is not an advantage to the internal serialization of the code I 
suggested.  However, just like pulling in the getenv() stuff from all of the 
callers of cli_gentemp, the added APIs simplify the calling code, whch is a 
good number of places.  An example is each place that calls cli_gentemp() to 
generate a directory name, and then calls mkdir().  Like:

          tempname = cli_gentemp(NULL);
          if(mkdir(tempname, 0700)) {
               cli_dbgmsg("ScanHTML - > Can't create temporary directory 
%s\n", tempname);
               return CL_ETMPDIR;
           }

is replaced by:
          if ((tempname = cli_gentempdir(NULL)) == NULL) {
               cli_dbgmsg("ScanHTML - > Can't create temporary 
directory\n");
               return CL_ETMPDIR;
          }

Note, that the original code is already broken (it needs to be changed to 
the NULL: return which cli_gentemp can return).

Attached is a patch which removes the remaining tmpfile() occurrances in 
libclamav.  While doing that it also fixes a few open file leaks in error 
paths.


["20041130.patch.new2" (application/octet-stream)]

_______________________________________________
http://lists.clamav.net/cgi-bin/mailman/listinfo/clamav-devel


[prev in list] [next in list] [prev in thread] [next in thread]