Trying to debug a c program segmentation fault using output from valgrind

Question

A CLI program of mine compiles and runs fine on windows. Compiles fine on linux, but causes a segmentation fault when running.

I turned to stackoverflow for help, and found a few questions similar to what I was going to ask that suggested valgrind, which I just happen to have installed (woo!).

So I ran my program through valgrind, and got a depressingly large amount of output, but I shall start with the first error message:

==11951== Command: ./vt
==11951== 
Loading...
Load default database? (y/n)y
Opened input file vtdb.~sv, reading contents...
==11951== Invalid write of size 1
==11951==    at 0x400FA9: readnumberfromfile (in /home/rob/Documents/programming/c/vocabtest/vt)
==11951==    by 0x400C21: getrecordsfromfile (in /home/rob/Documents/programming/c/vocabtest/vt)
==11951==    by 0x401FFD: main (in /home/rob/Documents/programming/c/vocabtest/vt)
==11951==  Address 0x53b05bb is 0 bytes after a block of size 11 alloc'd
==11951==    at 0x4C28FAC: malloc (vg_replace_malloc.c:236)
==11951==    by 0x400EAC: readnumberfromfile (in /home/rob/Documents/programming/c/vocabtest/vt)
==11951==    by 0x400C21: getrecordsfromfile (in /home/rob/Documents/programming/c/vocabtest/vt)
==11951==    by 0x401FFD: main (in /home/rob/Documents/programming/c/vocabtest/vt)
==11951== 
...finished.
1180 entries read from vtdb.~sv.

The problem seems to be in readnumberfromfile, and I've looked through it, and I can't seem to find what's wrong with it!

Can anyone shed some light?

int readnumberfromfile (int maxvalue,char separator)
{
    int number, i=0;
    char ch;
    char * buff = (char *)malloc(11);//allocate enough space for an 10-digit number and a terminating null
    if (!buff) {printf("Memory allocation failed!\n");return 0;}//return 0 and print error if alloc failed
    if (!maxvalue) maxvalue=MAXINTVALUE;

    ch=getc(inputfile);
    while (!isdigit(ch))
    {
        if (ch == separator||ch=='\n'||ch==EOF) {fprintf(stderr,"Format error in file\n");return 0;}//if no number found(reached separator before digit), print error and return 0
        ch = getc(inputfile);//cycle forward until you reach a digit
    }
    while (i<11 && ch!=separator && ch!='\n')//stop when you reach '~', end of line, or when number too long
    {
        buff[i++]=ch;
        ch = getc(inputfile); //copy number from file to buff, one char at a time
    }
    buff[i] = '\0';//terminate string
    number = atoi(buff)<=maxvalue ? atoi(buff) : maxvalue;//convert string to number and make sure it's in range
    free(buff);
    return number;
}

This is called from getrecordsfromfile if that's of any use:

void getrecordsfromfile(char * inputfilename,char separator)
{
    int counter = 0;
    struct vocab * newvocab;
    struct listinfo * newvocablist;
    if (!(inputfile = fopen(inputfilename, "r")))
    {
        printf("Unable to read input file. File does not exist or is in use.\n");
    }    
    else
    {
        printf("Opened input file %s, reading contents...\n",inputfilename);
        while (!feof(inputfile))
        {
            newvocab = (struct vocab *)malloc(sizeof(struct vocab));
            if (!newvocab)
            {
                printf("Memory allocation failed!\n");
                return;
            }
            else
            {
                newvocab->question=readtextfromfile(MAXTEXTLENGTH,separator);
                newvocab->answer=readtextfromfile(MAXTEXTLENGTH,separator);
                newvocab->info=readtextfromfile(MAXTEXTLENGTH,separator);
                newvocab->hint=readtextfromfile(MAXTEXTLENGTH,separator);
                newvocab->right=readnumberfromfile(1,separator);
                newvocab->counter=readnumberfromfile(0,separator);
                newvocab->known=readnumberfromfile(3,separator);

                switch (newvocab->known)
                {
                    case 0: newvocablist = &n2l;break;
                    case 1: newvocablist = &norm;break;
                    case 2: newvocablist = &known;break;
                    case 3: newvocablist = &old;break;
                }

                addtolist(newvocab,newvocablist);
                if (newvocab->question==NULL||newvocab->answer==NULL)
                {
                    printf("Removing empty vocab record created from faulty input file...\n");
                    removefromlist(newvocab,newvocablist,1);
                }
                else counter++;
            }
        }
        fclose(inputfile);
        printf("...finished.\n%i entries read from %s.\n\n",counter,inputfilename);
    }
    return;
}

Full source can be gitted from https://github.com/megamasha/Vocab-Tester

A couple of notes: I am trying to help myself, I have done my research, looked at similar questions and found out about valgrind myself.

I am still a relative beginner though, and while I appreciate solutions (WHAT to do to fix it), yet more useful is knowledge (HOW to fix or avoid it myself next time). I am here (and very keen) to learn.

I don't know if this is related to your problem, but `getc` doesn't return a `char`, it returns an `int`. — Oliver Charlesworth, Aug 19 '11 at 14:32
On a seperate note, you may have a memory leak for a badly formatted file. You need to free(buff) after fprintf(stderr,"Format error in file\n"); — Seth, Aug 19 '11 at 14:51
@Oli: What a misleading function name! I didn't know that. A char is basically one byte though, right? And an int is, how big? I've not encountered a problem with using getc to read text before (which it would appear is what I've been doing all this time) Should I be worried? Anyone? — M_M, Aug 19 '11 at 15:17
@Seth: Thank you - good point. Function duly modified. I should look at my readtextfromfile function for similar omissions. — M_M, Aug 19 '11 at 15:19
@M_M, getc returns an int because it needs to be able to return EOF in addtiona to all possible characters. EOF can't be a character, because if it were then you would never be able to store it in a file and read past it. — Shannon Severance, Aug 19 '11 at 17:01
See http://stackoverflow.com/questions/1782080/what-is-eof-in-the-c-programming-language for a bit of discussion about EOF. — Shannon Severance, Aug 19 '11 at 17:23

amit · Answer 1 · 2011-08-19T15:14:01.903

7

buff[i] = '\0';//terminate string

in here i == 11, since you allocated only 11 chars, and while condition ends when i=11.
so, you access a memory you did not allocate.

the behavior for this situation is not defined.

you can solve this by allocating one extra character on your malloc.

edited Aug 19 '11 at 15:14

answered Aug 19 '11 at 14:30

amit

175,853
27
231
333

it's not that windows is tolerant, it's another form of undefined behaviour – Flexo Aug 19 '11 at 14:32
It is exceedingly unlikely that Linux will produce a `SIGSEGV` for `p = malloc(11); p[11] = 0;` (except when using ElectricFence). I suggest removing that (bogus) part of the answer. – Employed Russian Aug 19 '11 at 14:45
@Employed Russian: I accepted your advise and editted the answer. thanks. – amit Aug 19 '11 at 14:49
To be a little more precise - `i == 11` when the line of input has a series of 11 or more digits. – Michael Burr Aug 19 '11 at 15:01
Thanks, I actually want to only accept 10 chars, so I'm using `char * buff = (char *)malloc(10+1);` and have changed `while (i<10 && ch!=separator && ch!='\n')` – M_M Aug 19 '11 at 15:06

score 3 · Accepted Answer · answered Aug 19 '11 at 14:56

int number, i=0;
...
while (i<11 ...

You are reading up to eleven digits for i = 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 and 10. And then trying to stick the \0 in twelfth slot buff[11].

It's called an "off by one error".

So the fix depends on what you want to change. If you want to accept 11 characters, change the malloc of buff. If you want to only accept 10, then change the while condition.

score 2 · Answer 3 · answered Aug 19 '11 at 14:34

2

Invalid write of size 1

You're probably writing a char

Address 0x53b05bb is 0 bytes after a block of size 11 alloc'd

You've only just overflowed something of size 11

Both in readnumberfromfile

This is suspiciously related (by the sizes):

char * buff = (char *)malloc(11);

This will be done with i = 11 after the loop, which is past the end of the allocation:

buff[i] = '\0'

As wormsparty says you can get valgrind to be more helpful, by getting debug symbols in your binary.

answered Aug 19 '11 at 14:34

Flexo

87,323
22
191
272

Thanks, well-structured answer. How should I implement -g? – M_M Aug 19 '11 at 15:05
1

It depends on how you're building it. If you have a Makefile you can add -g to `CFLAGS`. Often you can even do it like: `CFLAGS="-g" make myprog`. If you're using CMake or Autotools or CDT there are smarter ways to add the flag. If it's already built you'll want to clean it first using `make clean` so that everything gets rebuilt with the new option. – Flexo Aug 19 '11 at 15:08
It's a very simple program - only one .c file of about 600 lines. So far I've been making with `sudo make vt`, which has been calling `cc vt.c -o vt`. I'm sure they shouldn't, but makefiles have me baffled. – M_M Aug 19 '11 at 15:22
if it's simple enough doing `sudo CFLAGS="-g" make vt` should do the trick then – Flexo Aug 19 '11 at 15:25
I shall try that. Lucky for me, my weekend starts around now, so I shall give that a try probably next week. I shall select my accepted answer then too. – M_M Aug 19 '11 at 15:28

score 1 · Answer 4 · answered Aug 19 '11 at 14:31

1

For later, if you compile with -g, valgrind will show you exactly at which line the segfault happened.

answered Aug 19 '11 at 14:31

wormsparty

2,481
19
31

to what line should I add -g? So far I've been making with `sudo make vt`, which has been calling `cc vt.c -o vt`. – M_M Aug 19 '11 at 14:59
cc -g vt.c -o vt should be fine. You might want -Wall (warnings all) too. – Sam DeHaan Aug 19 '11 at 16:53

Trying to debug a c program segmentation fault using output from valgrind

4 Answers4