1

So, I am currently working on System programming for my Unix OS class. All that this program should do is read a binary file and output the lines to a CSV file. I feel like i'm almost done but for some reason I keep getting a segfault.

To clarify: fd1 = input file, fd2 = output file, numrecs = number of records from input file. Somewhere in main():

for(i=0;i<numrecs;i++){
    if((bin2csv(fd1, fd2)) == -1){
        printf("Error converting data.\n");
    }
}

int bin2csv(fd1, fd2){
    bin_record rec;
    char buffer[100];
    int buflen;
    strncpy(buffer,"\0", 100); /* fill buffer with NULL */
    recs = &rec;

    /* read in a record */
    if((buflen = read(fd1, &recs, sizeof(recs))) < 0){
        printf("Fatal Error: Data could not be read.\n");        
        return -1;
    }

   sprintf(buffer, "%d, %s, %s, %f, %d\n", recs->id, recs->lname, recs->fname, recs->gpa, recs->iq);
   printf("%s\n", buffer);
   write(fd2, buffer, sizeof(buffer));
   return 0;
}

The segfault is occurring on the line "sprintf(buffer, etc..);" however, I cannot figure out why that is happening.

This is the error gdb spits out:

Program received signal SIGSEGV, Segmentation fault.
0x0000000100000c87 in bin2csv (fd1=3, fd2=4) at bin2csv.c:25
25 sprintf(buffer, "%d, %s, %s, %f, %d\n", recs->id, recs->lname,
recs->fname, recs->gpa, recs->iq);

Hopefully this is enough info. Thanks!

16
  • 1
    Segmentation faults are caused by bad memory accesses, and since the only memory accesses on that line are recs->..., you can assume recs is not being properly initialized. Check if it's NULL right before, and if not, keep digging. Dec 16, 2015 at 6:43
  • Post definition of bin_record. Dec 16, 2015 at 15:50
  • strncpy(buffer,"\0", 100); /* fill buffer with NULL */ That will not fill the buffer with NUL characters as strncpy() copies a maximum number of bytes, but still stops copying when the first NUL character is encountered in the source. Dec 16, 2015 at 16:03
  • 1
    @AndrewHenle That was my gut reaction too, because this is a non-standard way to zero a buffer, but strncpy will zero-pad the remainder of the buffer until n bytes have been written.
    – paddy
    Dec 16, 2015 at 21:07
  • 1
    @paddy Yes indeed. strncpy() NUL pads the target string. Learn something new every day... :) Dec 16, 2015 at 21:20

4 Answers 4

3

It looks like recs is a pointer. You are reading bytes directly into that pointer, like reading a raw memory address from file:

read(fd1, &recs, sizeof(recs))

And then you start using it in the call to sprintf... BOOM!

There is actually no reason to use it at all (is it a global?)... Even though you initialised it by recs = &rec, and assuming you don't trash it, it still will not contain valid address outside of that function. That's because rec is a local variable.

So, just read directly into rec like this:

read(fd1, &rec, sizeof(rec))

And then on the sprintf line, you use rec.id instead of recs->id (etc).

3
  • This fixed the segmentation fault! Thanks for that information. Dec 16, 2015 at 20:35
  • if recs is a pointer, then this line: if((buflen = read(fd1, &recs, sizeof(recs))) < 0){ will fail because the second parameter of read() has to be a pointer to the input buffer, not a pointer to a pointer to the input buffer. in the OPs posted code, the variable recs is undefined Dec 16, 2015 at 22:59
  • Thanks for paraphrasing my answer.
    – paddy
    Dec 17, 2015 at 0:20
2

I see a few issues here:

  1. sprintf does nothing to prevent writing past the end of the string buffer. In fact it has no knowledge of the length of that buffer (100 bytes in your case). Since you have setup the buffer in the stack, if sprintf over-runs your buffer (which it could do with long first or last names or garbage strings as input) your stack will be corrupted and a seg fault is likely. You may want to consider including logic to ensure that sprintf will not exceed the amount of buffer space you have. Or better yet avoid sprintf altogether (more on that below)

  2. You are not handling end-of-file in the code provided. For end of file, read returns 0. If you pass bad pointers to sprintf, it will fail.

  3. The functions that you are using are the UNIX derived ones (part of POSIX but decidedly low level) that use small integers as file descriptors. I would recommend using the FILE * based ones instead. The I/O functions of interest would be fopen, fclose, fprintf, fwrite, etc. This would eliminate the need to use sprintf.

See this previous question for more information.

2
  • Part of the description of this particular homework was that we were not able to use FILE * or any f*() functions. Otherwise, I would have used those. Dec 16, 2015 at 20:18
  • @Jake Parham, fair enough. I endorse Rahim's suggestion below about using snprintf then to protect against buffer overruns. If you do not have to produce re-entrant code in this assignment (likely), I also suggest taking the buffer out of the stack by marking it static. The identifier will still be local to the function. Good luck on your assignment. Dec 16, 2015 at 21:07
1
if((buflen = read(fd1, &recs, sizeof(recs))) < 0){

Use <= 0 rather than < 0, else when the return value is 0, sprintf(buffer ... may seg fault as it tries to de-reference recs->id which has an uninitialized value.

0

You have some problems: 1) structure of bin_record. It has char[] and it is possible to overflow. 2) in sprintf you cannot set buffer max size. it is better to use snprintf like this:

 sprintf(buffer, 100, "%d, %s, %s, %f, %d\n", recs->id, recs->lname, recs->fname, recs->gpa, recs->iq);

3) to fill buffer with null us this:

memset (buffer,'\0',100);

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.