"*** stack smashing detected ***" with file read and writes

user_8141772 0
Viewed 220

I'm working with a C program that reads from a single file and uses sprintf to write that data into multiple files, I'm going wrong somewhere, but I don't really know where and that results in this error:

*** stack smashing detected ***

The code usable for reproduction is given here:

FILE * source=fopen("card.raw","r");// defines source I will read from

char array[512];
int active_read=0;
char * filename;
sprintf(filename, "%03i.jpg",i);
FILE *image=fopen(filename, "a"); //my understanding of sprintf to create a file

while(fread(array,sizeof(char *),512,source)==512) // if 512 characters can be detected
{
    if(array[0]==0xff && array[1]==0xd8 && array[2]==0xff && (array[3] & 0xf0==0))
    {
        if(active_read==1)
            fclose(image);
        active_read=1;
        sprintf(filename, "%03i.jpg",i);
        image=fopen(filename, "a");
        fwrite(array, sizeof(char *),512, image);
        i++;
    }
    else if(active_read==1)
        fwrite(array, sizeof(char *), 512, image);
}

I rand the code with my debugger(CS50 Debugger). And I found that the if condition is never even checked. It jumps from the while loop to the else if condition, never does anything and then leaves with the error.

cs50 pointers c file
3 Answers

Besides the problem with char * filename; that should be an array, e.g. char filename[64];, there is a problem here:

(array[3] & 0xf0==0)

How is that evaluated?

Is it ((array[3] & 0xf0)==0) or (array[3] & (0xf0==0)) ?

Checkout https://en.cppreference.com/w/c/language/operator_precedence and you see that == has higher precedence than &. Consequently, you do 0xf0 == 0 first. That's always false (aka zero) so your expression is always false. So the compiler is allowed to (and probably will) optimize the generated code so that the expression isn't evaluated at run time. Instead it always go directly to the else if part.

In other words - the compiler is allowed to treat your code as-if it was:

while(fread(array,sizeof(char *),512,source)==512) 
{
    if(active_read==1)
        fwrite(array, sizeof(char *), 512, image);
                      ^^^^^^^^^^^^^^
}

Edit: Also notice that this should be sizeof(char) or just 1 (as sizeof(char) is always 1) as noticed by @Jabberwocky in a comment.

BTW: Be careful with sprintf as it may overflow the destination buffer. Consider using snprintfinstead.

user_43864270

To add to @4386427's answer, if your char type is signed, the comparison array[0]==0xff will always be false, as seen here (also a pro tip: try to enable every single warning that your compiler offers).

The reason is that 0xFF is 255, which is promoted to an int, and then compared to a signed char which is also promoted to int, but has a possible range of values from -128 to 127.

To make sure that your code works properly if char is signed, either change the array declaration to use unsigned char, or cast to unsigned before comparison:

 if ( (unsigned char)array[0] == 0xff && 
      (unsigned char)array[1] == 0xd8 &&
      (unsigned char)array[2] == 0xff &&
      ((unsigned char)array[3] & 0xf0) == 0 )
 ...
user_698090

So it looked like the general notion of my if condition was causing problems:

if(array[0]==0xff && array[1]==0xd8 && array[2]==0xff)

Since I had been comparing a single character from a character array with a byte. I had changed the type of array to BYTE * and the problem was solved

user_81417720
Related