2

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.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
Mr. Johnny Doe
  • 99
  • 11
  • 4
    1) `filename` is not initialized so the first `sprintf` gives [UB](https://en.cppreference.com/w/cpp/language/ub) already. 2) `fread(array,sizeof(char *),512,source)` Why `sizeof(char *)`? The `array` is `char[]` so that should be `sizeof(char)` or simply `1`. – dxiv Sep 04 '20 at 07:11
  • 3
    `filename` should be an array, not a pointer. – Barmar Sep 04 '20 at 07:13
  • 2
    `char * filename;` -> `char filename[64];` – Support Ukraine Sep 04 '20 at 07:15
  • @4386427 Why 64? – Mr. Johnny Doe Sep 04 '20 at 07:16
  • 1
    @Mr.JohnnyDoe It was just to show the principle, i.e. select a number sufficiently high so that you never overflow the buffer. In "real" code, you should use `snprintf` – Support Ukraine Sep 04 '20 at 07:18
  • Also, even with all these changes, I still have a situation where the if condition is not entered – Mr. Johnny Doe Sep 04 '20 at 07:18
  • 1
    `active_read` is kind of strange. We can't see how it is initialized and it's only assigned the value 1. You need to posted the code where it is defined and initialized – Support Ukraine Sep 04 '20 at 07:19
  • it is set to 0 outside the loop. When I look at it with my debugger, the control checks while loop condition. Checks else if, doesn't enter the condition though. And keeps going on. But it never even checks to see if my if statement is correct or not – Mr. Johnny Doe Sep 04 '20 at 07:23
  • @Barmar, It doesn't *have* to be an array, but it should point to a large enough buffer. – HAL9000 Sep 04 '20 at 11:24
  • @HAL9000 It doesn't have to be, but there's no particular reason why it needs to be allocated dynamically, so it might as well be. – Barmar Sep 04 '20 at 15:45
  • @Barmar, I agree that having `filename` be an array would be a good idea in this case, but "should be" are strong words. – HAL9000 Sep 04 '20 at 18:08
  • @HAL9000 I didn't mean "must be" in this case. – Barmar Sep 04 '20 at 18:59

3 Answers3

6

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.

Support Ukraine
  • 42,271
  • 4
  • 38
  • 63
1

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 )
 ...
vgru
  • 49,838
  • 16
  • 120
  • 201
0

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

Mr. Johnny Doe
  • 99
  • 11