Opened 19 years ago
Closed 17 years ago
#1312 closed defect (fixed)
use of static string in msTmpFile() — at Version 15
Reported by: | Owned by: | mapserverbugs | |
---|---|---|---|
Priority: | high | Milestone: | 5.0 release |
Component: | MapScript | Version: | 4.5 |
Severity: | normal | Keywords: | |
Cc: |
Description (last modified by )
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.
Change History (15)
comment:2 by , 19 years ago
Agreed that it doesn't need to be static if you rework the function to initialize tmpId on every call.
comment:3 by , 19 years ago
Status: | new → assigned |
---|
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.
comment:4 by , 19 years ago
Oops, sorry. I overlooked that we were depending on static-ness. I did not make any other such changes.
comment:5 by , 19 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.
comment:6 by , 19 years ago
Cc: | added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
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.
comment:7 by , 19 years ago
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.
comment:8 by , 19 years ago
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.
comment:11 by , 19 years ago
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.
comment:12 by , 19 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
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)
comment:13 by , 18 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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.
comment:14 by , 18 years ago
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.
comment:15 by , 17 years ago
Description: | modified (diff) |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
Closing again. Please create new ticket with more specific details if there is really still a problem with this.
Note:
See TracTickets
for help on using tickets.