Problem in sprintf

[Greg White]

Hello everyone:

I have this function that works most of the time but sometimes returns garbage and sometimes even crashes my program. It's so simple, I don't see what could be causing the problem. I thought that maybe the string filename was too small but it still is broken when I make it a char[2000]. Any ideas?

char *tempfilename(i)
int i;
{
char filename[80];

sprintf(filename,"/tmp/T%d%d.tmp",getpid(),i);
return(filename);
}



Thanks in advance, Greg

[A. Clay Stephenson · Accepted solution]

Sure Greg, this is duck soup.

Change:
char filename[80];

to this:
static char filename[80];

and live happily ever after.

Regards, Clay

[Greg White]

Hi Clay,

That fixed my problem, sort of, but now no matter how many times I call this function, it always uses the value of the last call. Any previous calls are wiped out. The thing I don't understand is that this worked perfectly under 10.20 but fails under 11.0. Any more ideas?

Greg

[Volker Borowski]

Hi Greg,

in your case, the char[80] is located on a dynamic area, that is unprotected when the subroutine is left. So it might occurr, that the memory is used otherwise.

Way out one: declare static as described -> pitfall: a successive call might bring suprises like

c1=tempfilename(1);
c2=tempfilename(2);

guess what the contents of c1 will be.

Way out two: allocate memory dynamic inside you subroutine:

char *filename=malloc(80);

pitfall: the calling program needs to free the memory or you will have memory leaks.

Way out three (suggested):
let the calling routing get the memory and pass the storelocation in addition

char *tempfilename(i,s)
int i;
char *s;
{
sprintf (s,...
return s;
}

pitfall: will get SIGSEGV if s is not initialized before the call.

Hope this helps
Volker

[A. Clay Stephenson]

Well I know what's wrong but you are not going to like it. Your code was working by accident. You also need to fix your 10.20 version because again it is a pure crapshoot here depending upon the state of the stack.

Your fundamental problem is that in your original version, the variable was auto declared and thus was allocated from the stack. The present version is static declared and thus the value is safe but since thtere is now only one variable, whatever happens last wins. If you call the function 5 times, all five pointers have the same value. (When I saw your posting, I actually thought this might be the problem).

Plan A:
Round-robin buffer

char *tempfilename(i)
int i;
{
#define MAX_ROWS 6
static int i = -1;
static char filename[MAX_ROWS][80];

++i;
if (i >= MAX_ROWS) i = 0;
sprintf(filename[i],"/tmp/T%d%d.tmp",getpid(),i);
return(filename[i]);
}

Plan B:
Dynamic Allocation:

char *tempfilename(i)
int i;
{
char filename[80],*p = NULL;
int len = 0

sprintf(filename,"/tmp/T%d%d.tmp",getpid (),i);
len = (int) strlen(filename) + 1;
p = (char *) malloc((size_t) len);
if (p != NULL)
{
p[0] = '\0';
strncat(p,filename,len);
}
return(p);
}


In the round-robin version, the space will not be re-used until you exceed MAX_ROWS and then it will srap back to 0. In the dynamic version, you should call free() to return the memory to the heap when you are finished with the pointers.



Regards, Clay