Segmentation Fault on fclose
 

Hi,

I've been writing C code for 7 years in a SCO Openserver environment. I'm currently migrating this code from SCO to Red Hat EL Server release 5. I've gotten a lot of SIGSEGV errors along the way, mostly because the C compiler in SCO lets you get away with lazy programming. :o

In any case, I've fixed most with better error checking and initialization. I've come across one though, that I've had difficulty cracking. The app basically opens a file, reads the first three characters to a variable for processing (this works), then reads the rest of the file, which may or may not be populated, processes this information and then closes the file. At file close, the application dies with a segmentation fault.

Here is the code in question:

Here is the output from the printfs:

And finally, here is the 'bt full' results:

Any help would be greatly appreciated, as I'm a RedHat noob. Please let me know if further information is required. Thanks in advance.

Hi -

A couple of suggestions:

1. Anything that trashes memory; any buffer overrun in the program between "fopen()" and "fclose()", could conceivably cause "fclose()" to fail. So (as you're probably already aware), "fclose()" (or, for that matter, *any* of your I/O calls) could be innocent victims of the actual root cause.

2. You might try something like "valgrind" or "Electric Fence":
http://valgrind.org/info/
http://www.cprogramming.com/debugging/valgrind.html
etc etc

3. You should definitely substitute "fprintf (stderr)" for "printf()/fflush (stdout)". Less clutter, better results. You'll like it - promise!

4. I don't see where you're sanity-checking "glbcfp->maxtrkcp", "glbcfp->maxcllp" and friends.

5. Similarly, I don't see where you're checking if the loop goes out of bounds. Is there any chance "tp" might fall outside of the buffer you've allocated for "tbuff"? Or any of your reads might be larger than tbuff itself?

6. You check for "fp = fopen() == NULL" (good...), but then later on you qualify "fclose()" with "if (fp) ...". Haven't you already verified that fp *ISN'T* null?

For that matter, if you're worried about fp (and it's a good thing to worry about!), why not always set "fp = NULL" immediately after every "fclose()"?

7. Also, explicitly set every pointer "*p = NULL" after every free.

It's cheap insurance, and it's a good practice.

8. A quick'n'dirty workaround might be to just declare "tbuf[]" as a big, huge array (instead of worrying about malloc ... and worrying if you've malloc'ed enough). You still need to make sure you don't read out of bounds, but it couldn't hurt...


IMHO .. PSM

I was about to suggest valgrind; but, I was beaten to the punch.

My only other point is that you open the file as a stream (fopen) and read from it using read() as opposed to fread(): fread() is the preferred method to read a stream)

I've always though it best to stick with one or the other.

I found this:

Family open(), close(), read() and Co:
- are not C standard.
- are Unix, BSD, POSIX and Single Unix Specification standard compliant.
- are wrappers to syscalls.
- are not formatted I/O : we have a unformatted block of one or more bytes.
- don't use a C standard I/O buffer.
- use Linux VFS buffers and cache.
- generally used for accessing data at a low level (device or raw
filesystem format).

Family fopen(), fclos(), fread() and Co:
- are C standard.
- are functions of the standard I/O libc (glibc).
- use an internal buffer (in their coding).
- enable formatted I/O on some calls.
- use a C standard I/O buffer.
- use Linux VFS buffers and cache.
- are generally used for accessing data streams in a device independant
fashion.

I also found this line curious

FILE *fp,*fopen();

fopen() is defined in stdio.h, you shouldn't declare it yourself.

Everything here is very helpful. I am currently looking to install valgrind and do some good testing. I am also investigating the fread command. I will also try removing the fopen declaration. I will update as soon as I have some answers. Again, thanks for the assistance. This has been driving me nuts. Reading through the debug statements, it seems as though everything is going through ok, but looks can be deceiving.

Update: sort of. I'm awaiting a standalone server, because Valgrind locks up my virtual server and it's loaded down so there are no more resources available to re-allocate.

You've got some crazy defines and/or typedef's going on there. ;)

Thanks! Most were inherited. This code was originally written for Xenix in the early 90s. :D

Update: got a stadalone for a few days, installed valgrind and ran a valgrind --tool="memcheck" -v and right after the read() got the following error:

So it sounds like the malloc didn't work properly or at least read doesn't like it very much. Ironically, the segmentation error didn't occur.

Hi -

First, please consider changing your "read()" and your "printf ()/fflush()" calls to something like this:Second, please verify that tbuff is successfully allocated, and is always at least as large as glbcfp->maxtrkcp. Adding more "fprintf()" statements is arguably the fastest/easiest way to do this.

Third, keep an eye on code like:
I didn't notice you explicitly writing to "tp" (I didn't look very closely), but it definitely looks suspicious. Again: adding more "fprintf()'s" might help.

I've already started transitioning to fprintf. These are just debug statements that will be removed post haste. I have already changed read() to use fread().

Tp is only pointed to in order to see if the corresponding trunk is already assigned. The assignment to *tp is done in a different section of the code once the assignement is completed successfully and is completely independent from this executable.

The check for length of read is done in the if statement in which the read takes place, although the value is not saved to a variable:

I apologize. I forgot to update this thread. Apparently, for whatever reason, read/fread did not like reading into a pointer. Doing so caused FILE *fp to be overwritten and fileno(fp) was returning -1, rather than 6. I changed tbuf from "char *tbuf;" to "char tbuf[glfcfp->maxtrkcp];" and the problem went away.

Valgrind was a great help, and has identified a great number of memory leaks from the last 20 years that I have been able to fix without finding them in a production site, so thanks. Also, thanks for all the pointers (no pun intended). Much appreciated.

Just a general comment: add setlinebuf(stdout); after your declarations and get rid of all of the fflush(stdout);.

Also, your function declaration FILE *fopen(); isn't any good, so your use of FILE *fopen(const char*, const char*); probably defaults to int fopen(int, int);. Not really a big deal in this particular case, but try the appropriate declaration instead.

Should have read your last post before looking at your code for 30 minutes! Glad you fixed it.
ta0kira