CS50 recover segmentation fault

Question

This is Recover from pset4 CS50. There is no error when I compile it. When I run the code I get segmentation fault. I do not understand what exactly is the mistake. I have looked up solutions related to questions on segmentation fault posted by others but they don't seem to solve my problem.

Can somebody explain whats wrong and how do I solve it.

#include <stdio.h>
#include <stdint.h>
typedef uint8_t BYTE;

int main(int argc, char *argv[])
{

if (argc != 2)
{
    printf("Usage: ./recover image\n");
    return 1;
}

FILE *inFile = fopen(argv[1], "r");
if(!inFile)
{
    printf("Could not open file!\n");
    return 1;
}

BYTE buffer[512];
FILE *outFile = NULL;
int imageNum = 0;
char fileName[8];

while (!feof(inFile))
{
     fread(buffer, 1, sizeof(buffer), inFile);
     if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[4] & 0xf0) == 0xef0)
     {
         if (imageNum > 0)
         {
            fclose(outFile);
         }
         imageNum++;
         sprintf(fileName, "%03i.jpg", imageNum);
         outFile = fopen(fileName, "w");
     }
    if (outFile != NULL)
    {
        fwrite(buffer, 1, sizeof(buffer), outFile);
    }
}
fclose(outFile);
fclose(inFile);
return 0;
}

`fwrite(buffer, 1, sizeof(buffer), outFile);`... Did you mean: `fwrite(buffer, sizeof(buffer), 1, outFile);`? ( Definition of [fwrite()](https://www.tutorialspoint.com/c_standard_library/c_function_fwrite.htm) ) — ryyker, Jun 02 '20 at 13:22
Read and understand this: [**Why is “while ( !feof (file) )” always wrong?**](https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) — Andrew Henle, Jun 02 '20 at 13:46
Thank you for your help. My while loop condition was partially the problem. It has now been resolved — Qunoot K, Jun 04 '20 at 07:58

ryyker · Answer 1 · 2020-06-02T15:57:59.397

1

The reason for the segfault is likely this declaration:

char fileName[8];

For filenames greater than 999, fileName will overflow.

i.e. for imagenum >= 1000

sprintf(fileName, "%03i.jpg", imageNum);//produces 8 characters + NULL == 9

...will produce "1000.jpg" which is a buffer overflow, thus segfault.

Making fileName larger is the fix:

char filename[20];//or larger as needed.

Also,
per definitions of fwrite() and fread(), the following function arguments are misplaced:

fread(buffer, 1, sizeof(buffer), inFile);
fwrite(buffer, 1, sizeof(buffer), outFile);
               ^        ^

Should be:

fread(buffer, sizeof(buffer), 1, inFile);
fwrite(buffer, sizeof(buffer), 1, outFile);
                     ^         ^

edited Jun 02 '20 at 15:57

answered Jun 02 '20 at 13:29

ryyker

22,849
3
43
87

I changed the size of my fileName and also the fread() and frwite() functions according to what you mentioned but I am still getting the segmentation fault – Qunoot K Jun 03 '20 at 13:54
@ZaraK - More information about exactly where the fault occurring would be helpful. Have you removed `while (!feof(inFile))` from the code yet. (see comment under your post.) Are you running in a debugger? ([gdb](https://www.gnu.org/software/gdb/)) – ryyker Jun 03 '20 at 14:21
Thank you for your help. I ran debug 50 and found that the issue was with my if condition.. – Qunoot K Jun 04 '20 at 07:59

score 1 · Accepted Answer · answered Jun 04 '20 at 08:17

I found the solution to problem above by changing these lines of code

while (!feof(inFile))

to

while (fread(buffer, sizeof(buffer), 1, inFile))

and

fread(buffer, 1, sizeof(buffer), inFile);
fwrite(buffer, 1, sizeof(buffer), outFile);

to

fread(buffer, sizeof(buffer), 1, inFile);
fwrite(buffer, sizeof(buffer), 1, outFile);

Also the main issue was in my if condition which I changed from

if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[4] & 0xf0) == 0xef0)

to

 if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)

and the last change

imageNum++;
sprintf(fileName, "%03i.jpg", imageNum);
outFile = fopen(fileName, "w");

to

sprintf(fileName, "%03i.jpg", imageNum);
outFile = fopen(fileName, "w");
imageNum++;

Thank you all for your suggestions and help.

It is good you were finally able to work out the solution! The way you can let others know this question is resolved is to mark one of the answers posted as the accepted answer (hollow check mark) Yours is the most complete answer, so marking it would be appropriate :) — ryyker, Jun 04 '20 at 12:24

score 0 · Answer 3 · answered Jun 02 '20 at 13:28

0

fileName is defined to have eight elements in char fileName[8];, but sprintf(fileName, "%03i.jpg", imageNum); writes more than eight characters once imageNum exceeds 999.

answered Jun 02 '20 at 13:28

Eric Postpischil

195,579
13
168
312

CS50 recover segmentation fault

3 Answers3

Linked