What is causing this segfault?

Question

I get a segfault after calling mygets(). Here is my code:

    #include <termios.h>
    #include <stdio.h>
    #include <stdlib.h>
    
    typedef struct _charlist{
        char ch;
        struct _charlist *next;
    }charlist;
    
    static struct termios old, new;
    void mygets(char** p);
    char getche(void); 
    char getch_(int echo);
    void resetTermios(void);
    void initTermios(int echo);
    
    
    int main(void){
        char *p;
        
        printf("\nWho loves orange soda?: ");
        mygets(&p);
        printf("'%s' loves orange soda!", p);
    }
    
    
    void mygets(char** p){
        char c;
        charlist *root, *curr;
        unsigned i, count=0;
    
        root=NULL;
    
    while((c = getche()) != '\n'){      
    
        count++;    
        if(!root){ 
            root = (charlist*) malloc(sizeof(charlist));
            root->ch = c;
                root->next = NULL;
                curr = root;
            }
    
            else{
                curr
->next = (charlist*) malloc(sizeof(charlist));
                curr->next->ch = c;
                curr->next->next = NULL;
                curr = curr->next;
            }
        }
    
        //now request string space.
        *p = (char*) malloc((count+1)*sizeof(char));
    
        printf("\np is now malloced");      //This line doesn't get printed!
        
        //move linked list into string space.
        curr = root;
        for(i=0; i<=count; i++){
            *p[i] = curr->ch;
            curr = curr->next;
        }
    
        //null-terminate the string.
        *p[i] = '\0';
    
    }

Can someone tell me why I get a segfault?

I can't post this question unless the ratio of code to question is lower than some arbitrary threshold. Therefore, there now follows the first paragraph of Alice in Wonderland, for your consideration.

Alice was beginning to get very tired of sitting by her sister on the bank, and of having nothing to do: once or twice she had peeped into the book her sister was reading, but it had no pictures or conversations in it, 'and what is the use of a book,' thought Alice 'without pictures or conversation?'

`func()` does not affect `p` in `main()`. `printf("'%s'", p);` is UB. — chux - Reinstate Monica, Aug 20 '14 at 23:02
Please provide a stack trace from a debugger session. (And side note: if you can't guarantee that count is "small" i.e. when count is provided by a user make sure to check for an overflow in the multiplication) — johannes, Aug 20 '14 at 23:02
In addition to Alexander's answer, you should not cast the result of malloc: http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc — IllusiveBrian, Aug 20 '14 at 23:03
@johannes Since `sizeof(char)` is 1, `sizeof(char)*(count+1)` has little change of overflowing due to multiplication. — chux - Reinstate Monica, Aug 20 '14 at 23:04
You need to post the code that "gives a string" to `p`, as it may be causing your segfaults. — Drew McGowen, Aug 20 '14 at 23:26
@DrewMcGowen If I put a printf("I'm here!") after the line I suspect, it doesn't print. I get a segfault error instead. That's how I know the segfault happens there. — Korgan Rivera, Aug 20 '14 at 23:28
This code is either incomplete or plain wrong, in both versions. Why should printf do anything but fault given a pointer to a raw block of memory? — david.pfx, Aug 20 '14 at 23:29
@david.pfx The segfault happens before it returns to printf(). — Korgan Rivera, Aug 20 '14 at 23:34
@KorganRivera: UB is shorthand for the dreaded [Undefined Behavior](https://stackoverflow.com/q/2397984). Please read that Q&A carefully. BTW: Do not use `sizeof(char)`, just write 1, if anything at all. — Deduplicator, Aug 20 '14 at 23:47
@Deduplicator TIL UB. I was just using sizeof in case magic happened, and char became several bytes. :) — Korgan Rivera, Aug 20 '14 at 23:57
@KorganRivera: Well, if you have UB, 1 may be 2, sure ;-) Still, I think time-travel is funnier: http://blogs.msdn.com/b/oldnewthing/archive/2014/06/27/10537746.aspx — Deduplicator, Aug 21 '14 at 00:01
@KorganRivera as @chux mentioned, `sizeof(char)` is *always* 1. No matter what. — Drew McGowen, Aug 21 '14 at 00:01

score 4 · Answer 1 · answered Aug 20 '14 at 23:02

4

When func is called, it is passed a copy of the local variable p in main. This copy is then assigned the malloced area in func. The original p in the main is never modified, so its contents remain undefined, causing a segmentation fault when printf dereferences p in order to print the string.

You may want func to return a char* pointing to the newly malloc'd area.

answered Aug 20 '14 at 23:02

Alexander Gessler

45,603
7
82
122

Noteworthy: the `char *p` in `void func(char* p)` does not do much useful here. – Jongware Aug 20 '14 at 23:04
So to change the pointer in main() I would need to pass a pointer to the pointer? – Korgan Rivera Aug 20 '14 at 23:12
I think Vlad's response even has a code sample for it? – Alexander Gessler Aug 20 '14 at 23:46
@AlexanderGessler That's what I used. – Korgan Rivera Aug 21 '14 at 00:35

score 1 · Answer 2 · answered Aug 20 '14 at 23:06

1

You pass the argument to the function by value. So according to the function declaration

void func(char* p);

parameter p is a local variable of the function that will be destroyed after exiting the function. Any changes of the local variable do not influence on the argument.

You could define the function the following ways

char * func(){
    unsigned count = 10;
    char *p = (char*) malloc(sizeof(char)*(count+1));

    //p is given a string after this, but problem is the above line.

   return p;
}

and call it as

p = funct();

or

void func(char ** p){
    unsigned count = 10;
    *p = (char*) malloc(sizeof(char)*(count+1));

    //p is given a string after this, but problem is the above line.
}

and call it as

func( &p );

answered Aug 20 '14 at 23:06

Vlad from Moscow

301,070
26
186
335

Your second example was what I was trying to do. Thanks! :) – Korgan Rivera Aug 20 '14 at 23:13
Did you zero-terminate the string? – Alexander Gessler Aug 20 '14 at 23:55
@AlexanderGessler Yep! But like I said, the segfault happens before I do anything with p. That's what's confusing me. – Korgan Rivera Aug 20 '14 at 23:59
1

Even better would be `*p = malloc( sizeof **p * (count + 1));` – John Bode Aug 21 '14 at 00:01

M.M · Accepted Answer · 2014-08-21T01:07:10.560

The problem is with:

*p[i] = curr->ch;

Should be:

(*p)[i] = curr->ch;

You want to access the i'th character of where p is pointing to. Not dereference the ith pointer in an array of pointers.

Same problem with *p[i] = '\0'; later.

Also you did not malloc enough space, as your loop writes count + 1 characters and then you write an extra null terminator, so you should either malloc count + 2 or adjust your loop to finish at i<count, not i<=count. (probably the latter).

Also, it'd be useful to check curr != NULL before dereferencing it, so that if you do have an off-by-one error then you don't get undefined behaviour.

What is causing this segfault?

3 Answers3