search

MapServer / MapServer-import

3 stars 2 forks source link

use of static string in msTmpFile() #1312

Open tbonfort opened 12 years ago

tbonfort commented 12 years ago

Reporter: sgillies@frii.com Date: 2005/04/13 - 02:00

In msTmpFile():    

    static char tmpId[128]; /* big enough for time + pid + ext */

I'm pretty sure that this doesn't need to be static.  Am going to make the change.
I appreciate comments or feedback.
tbonfort commented 12 years ago

Author: sgillies@frii.com Date: 2005/04/13 - 02:01

setting target to 4.6.
tbonfort commented 12 years ago

Author: dmorissette Date: 2005/04/13 - 03:00

Agreed that it doesn't need to be static if you rework the function to
initialize tmpId on every call.
tbonfort commented 12 years ago

Author: dmorissette Date: 2005/06/24 - 15:22

Sean,

I see in the code that you already removed the static in maptuil.c r1.178, but
you didn't adapt the code to properly initialize tmpId in all calls to the
function. As a result the msTmpFile() function is currently broken if it is
called more than once.

We'll fix this one, but if you removed other static instances elsewhere then
please review them or let us know where they are so that we can make sure that
the fixes won't have caused side-effects.
tbonfort commented 12 years ago

Author: sgillies@frii.com Date: 2005/06/24 - 15:45

Oops, sorry. I overlooked that we were depending on static-ness. I did not make
any other such changes.
tbonfort commented 12 years ago

Author: assefa Date: 2005/06/24 - 15:50

I have updated the function to always initialize the tmpid (in both cvs head 
and 4.6 branch). It seems to work with my test data.
Marking it as Fixed. If any problems please reopen.
tbonfort commented 12 years ago

Author: fwarmerdam Date: 2005/07/27 - 17:11

It seems various changes resulted in the counter always being set to 0, 
and to the forcebase logic not working at all.  I have updated the code
to restore these functionalities.  

Note that as the code was left, the counter was always zero, so two WMS 
requests in the same second would result in the same tmpfile name being
generated. 

I have only made this changes in 4.7 (cvs) so far, but it looks like it is
broken in 4.6 as well.   I am reopening so folks can review before I backport
the change into 4.6.
tbonfort commented 12 years ago

Author: fwarmerdam Date: 2005/07/27 - 17:12

Current implementation:

char *msTmpFile(const char *mappath, const char *tmppath, const char *ext)
{
    char *tmpFname;
    char szPath[MS_MAXPATHLEN];
    const char *fullFname;
    char tmpId[128]; /* big enough for time + pid + ext */

    if( ForcedTmpBase != NULL )
    {
        strncpy( tmpId, ForcedTmpBase, sizeof(tmpId) );
    }
    else 
    {
        /* We'll use tmpId and tmpCount to generate unique filenames */
        sprintf(tmpId, "%ld%d",(long)time(NULL),(int)getpid());
    }

    if (ext == NULL)  ext = "";
    tmpFname = (char*)malloc(strlen(tmpId) + 4  + strlen(ext) + 1);

    sprintf(tmpFname, "%s%d.%s", tmpId, tmpCount++, ext);

    fullFname = msBuildPath3(szPath, mappath, tmppath, tmpFname);
    free(tmpFname);

    if (fullFname)
        return strdup(fullFname);

    return NULL;
}

Note, that I also change tmpCount to initialize to 0 instead of -1 
since we no longer need a "not initialized" marker.
tbonfort commented 12 years ago

Author: assefa Date: 2005/07/27 - 19:56

Frank,

 From what I can see it adresses the previous issue described in comment #3, 
as well as correcting the bug you described. So I see no reason not to port it 
to 4.6.1

Thanks for fixing it.
tbonfort commented 12 years ago

Author: fwarmerdam Date: 2005/07/27 - 20:34

OK, it sounds like Sean is going to back port it.
tbonfort commented 12 years ago

Author: sgillies@frii.com Date: 2005/07/27 - 21:11

Tests pass, and now it's in the 4.6 branch as well.
tbonfort commented 12 years ago

Author: szekerest Date: 2005/08/21 - 00:27

But this problem exists in the current release (4.6.0), is it. I've run into 
the same problem: mapserver couldn't create the temp file, the file name was 
invalid because of the uninitialized tmpId on the second call.
tbonfort commented 12 years ago

Author: dmorissette Date: 2005/08/21 - 01:28

Sean's fix was made after the 4.6.0 release and will be in 4.6.1. You can also
checkout CVS branch "branch-4-6" to get it.

Marking bug as FIXED (it was left with status REOPENED)
tbonfort commented 12 years ago

Author: cavallini@faunalia.it Date: 2006/04/05 - 08:36

When mapscript is loaded as dl it works, because pid is 
always different, and timestamp is correctly updated, but when it is loaded 
statically in php.ini it does not work, because timestamp is fixed at the time 
of mapscript loading and obviously pid is always the same; the counter does 
not resolve the ambiguity.
tbonfort commented 12 years ago

Author: fwarmerdam Date: 2006/04/05 - 15:39

Paulo,

I don't follow your point.  The code uses a counter that increments for each
call to msTmpFile() so in the case of long running PHP instance all the temp
file will have the same <timestamp><pid> portion of the filename, but the
counter will increment keeping each of the requested msTmpFile() name requests
distinct. 

Have you actually seen a problem?  Can you see some reason why the 
incrementing counter doesn't make things unique?

The timestamp and pid stuff is intended to keep the files of different processes
distinct since they all start their counters from zero.
tbonfort commented 12 years ago

Author: dmorissette Date: 2007/08/08 - 20:23 Closing again. Please create new ticket with more specific details if there is really still a problem with this.