Thread: Seg fault on string pointer

Hi all,
I'm new to this board. I've had some forgotten experience with C, but now I'm refreshing.

I've always viewed string input in C as a great limitation on my programmes, because you have to know the lenght of the string beforehand, so I thought why not write a function that will take care of the memory allocation for me? Here's the code I wrote for the function strin, I also wrote a complete programme (with main()) just to test it.

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
                                                                                
char* strin(FILE* stream);
                                                                                
int main (void)
{
        /* variable declaration */
        char* p;
                                                                                
        /* Input */
        printf("\nEnter a line of text: ");
                                                                                
        if((p = strin(stdin)) == NULL)
        {
                fprintf(stderr, "\nstrin:String Input Failure.\n\n");
                exit(1);
        }
                                                                                
        puts(p);
                                                                                
        return 0;
}

char* strin(FILE *stream)
{
        char p[1];
        char *t;
        int  ch;
        int ind = 0;
                                                                                
        while((ch = fgetc(stream)) != '\n')
        {
                if((t = realloc(p, sizeof(char) + strlen(p))) == NULL)
                        return NULL;
                p[ind++] = (char)ch;
        }
        p[strlen(p)] == '\0';
                                                                                
        return p;
}

Like I said, I'm refreshing my knowledge of C and for things like the declaration of p[] in strin() I switched between many forms like
char *p
char p[]="";
etc.

I know that even without the segfault there's a lot wrong with the programme, but for now I just want to get it running even if it doesn't do what I intend it to do. I'll take care of other problems later.

My understanding is that because p is a name of a string in strin(), then it is a pointer as well However, based on what I understand from GDB, the segfault occurs on

Code:

 p[ind++] = (char)ch;

Any help in solving this seg fault would be appreciate.

> char p[1];
1. it isn't big enough
2. you're passing it to realloc

Try
static char p[BUFSIZ];

and ditch all the realloc stuff for now.

>p[strlen(p)] == '\0';
Why are you comparing?
Even if you meant assignment, its still wrong, because p[strlen(p)] is always \0 anyway (that's what strlen() is all about)

p[ind++] = '\0';
is what you want to end the string properly

Code:

        char p[1];
        char *t;
        int  ch;
        int ind = 0;
                                                                                
        while((ch = fgetc(stream)) != '\n')
        {
                if((t = realloc(p, sizeof(char) + strlen(p))) == NULL)
                        return NULL;
                p[ind++] = (char)ch;
        }
        p[strlen(p)] == '\0';

With the given declartion of p, it can only ever hold one single character. Don't bother with an array, just use a pointer. Especially since all you're doing is reallocing to it anyway.

[edit]Curses, foiled again.[/edit]

Quzah.

Thank you guys for those quick responses.

Salem, what do you mean drop the realloc stuff? What programme would I have without realloc?

quzah, if I declare p as [bold]char *p;[/bold], will I still be able to manipulate it as a string? ie. p[ind] = (char)ch; ?

Thanks for your time.

>>What programme would I have without realloc?
One that uses a static sized buffer.

>>will I still be able to manipulate it as a string
Yes, once you've pointed it at a valid memory area.

Yes. It's essentially the same thing.

Code:

char *p = "Hi!";
putchar( p[0] );
putchar( 1[p] );
putchar( *(p+2) );

[edit]Curses, foiled again![/edit]


Quzah.

Originally posted by quzah

Code:

char *p = "Hi!";
putchar( p[0] );
putchar( 1[p] );
putchar( *(p+2) );

I can hear Cikotic's brain crying out in pain already

Here's a reworked version of the original code.

Code:

char *strin(FILE *stream)
{
  char  *p = NULL;  /* NULL forces realloc() to act 
                     * as malloc() the first time its used */
  char  *t;
  int   ch;
  int   ind = 0;    /* Length of the starting string */

  while ((ch = fgetc(stream)) != '\n' && ch != EOF)
  {
    /*
     * realloc current string length +2 bytes,
     * 1 for the new char
     * 1 for the terminating \0 to be applied later
     */
    if ((t = realloc(p, ind+2)) == NULL)
    {
      /*
       * breaking here allows for a more controlled return
       * to the calling function, and doesn't cause
       * a memory leak if p already points to valid memory
       */
      break;
    }
    /*
     * we must now reassign t back to p
     * and store the new char
     */
    p = t;
    p[ind++] = ch;
  }

  if (p)
  {
    /*
     * Providing p is pointing to valid memory, 
     * apply the \0
     */
    p[ind] = '\0';
  }

  /*
   * p might be NULL.
   * The calling function must free() the memory
   * pointed to by p
   */
  return(p);
}

WOW. This is a great board, I'm overwhelmed with responses.

Hammer, I thought static makes a var's value the same throughout different calls to the function. what do you mean by a static sized buffer?
I'll have to check what static does later. For now, this is my new strin():

Code:

char* strin(FILE *stream)
{
        char *p = NULL;
        char ch[1];
                                                                                
        while((ch[1] = fgetc(stream)) != '\n' && (ch[1] != EOF))
        {
                if((p = realloc(p, sizeof(char) + strlen(p))) == NULL)
                        return NULL;
                strcpy(p, ch);
        }
                                                                                
        return p;
}

The output is still the same and based on GDB, the problem is at strlen(p) now, and I can understand why: the first time in the loop, p is not a string, just a NULL pointer. Do you think I sould make an if statement to separate the first realloc from every one after ( ie. if it's the first time in the loop, don't add strlen(p) to realloc's second arg, else add it)or is there a better way?

Thanx alot

>>Do you think I sould make...
No.

>>is there a better way
Yes, see my code in my previous post. There's no sign of that dreaded strlen() function

>>I thought static makes a var's value the same throughout different calls to the function.
Essentially yes, a static variable stays in existance after a function has terminated, so that upon re-entry you can continue using its previous value. I don't think you have any use for those in you program at present.

>>what do you mean by a static sized buffer?
"static sized" is where you've hard coded the array length:
char p[100]; /* 100 byte array */
You cannot call realloc() on an array name.
This type of variable is easier to work with, and is well suited to a beginner learning about arrays. What you're trying to do in your original program is dynamic memory allocation, where you make the array at run-time, creating it the exact size your program needs. This is more efficient in memory terms, but causes extra overhead in code writing and running.

Whether you choose a static array or dynamic is up to you. In your case, it simply depends on what you're trying to learn.

And now I've gone back to read your first post again, I can see you want dynamic memory. Maybe I'll stop rambling now ...

Thanx a lot, Hammer, your code makes everything clear now.
But one last thing. You said

Code:

 ... t = realloc(p, ind+2) ...

one space for the new char and one for the terminating NULL character. But, if I'm not mistaken, your code makes one space for a null character every time a new char is inputted, meaning that it'll end up with twice the memory it really needs. That is why I wanted an if statement to separate the first realloc() from every other one.

Correct me if I'm wrong.


Thanx again

Oh, I just remembered. The second argument of realloc() is the total size that we want for the array not the amount of memory to be added, which means your code is perfect, and I, once again, am in the wrong.

Thanx for the help.

I see you've worked it out now I was going to suggest you debug it yourself like so:

Add the following line just before the call to realloc()

printf ("We're about to request %d bytes of memory\n", ind+2);

and then you'll see what's happening.

But you know already.

OK, now Cikotic's goin' really psycho.
Take a look at this code from a different programme but with a similar concept:

Code:

 char* cc = "blah blah blah";
        char* op = " -o ";                      /* for output command */
        char* com;                              /* for the complete command */
        int tll;
                                                                                    
        tll = ((strlen(cc) + strlen(op)) + (2*strlen(argv[1])));
        
        if(argc < 2)
        {
                puts("BAD USAGE\n");
                return 0;
        }
        com = realloc(cc, tll + 2);  /* SegFault happens here. */

Is this code not 99.9% like the previous strin? Well, it's output is definitely the same: Segmentation Fault. There is obviously something about realloc() that I don't understand and that is causing me problem everytime I use it. Or maybe it's pointers. Anyway, if anyone sees a pattern in this and my previous problem, please tell me what I'm not understanding. I've almost memorized my C book's section on realloc and gone over it's code many times. I can't understand what I'm doing wrong.

Thanks for your time.

> com = realloc(cc, tll + 2); /* SegFault happens here. */
Yeah, the problem is you haven't read the manual

The first parameter should be either
- NULL
- the return result of some previous malloc / calloc / realloc call

You start with
char* cc = "blah blah blah";
which is neither of those conditions.

So naturally, what happens next is anyones guess. You drew the short straw and got a segfault (lucky you).

You only need
com = malloc ( ttl + 2 );