CODEWITHSUNDEEP
cstringalgorithmpointerssegmentation-fault
redsoxlost
redsoxlost

Reputation: 1235

Getting "Segmentation fault Core Dumped Error " while reversing a string

I am trying to learn C, I am getting this error while reversing a string. I am not well versed with memory allocation stuffs, can you please point out where I am doing wrong. 

#include<stdio.h>
#include<string.h>
char* strrev(char *str);
int main()
{
  char str[1000];
  char *str1;
  printf("Please enter a String\n");
  gets(str);

  str1=strrev(str);
  puts(str1);

}

char* strrev(char *str)
{
  char *str1;
  int i,length,c;
  length=strlen(str);

  for (i=length-1;i<=0;i--)
  {
     *(str1+c) = *(str+i);
     c++;
  }
  *(str1+c) ='\0';
  return str1;

}

Upvotes: 2

Views: 237

Answers (5)

Sourav Ghosh
Sourav Ghosh

Reputation: 134356

Inside your strrev() function, str1 is not allocated memory. Hence, *(str1+c) = *(str+i); is UB.

Then, c is an automatic local variable which is not initialized before use. Also UB.

Next, as Giorgi mentioned, correct your for loop.

That said, don't use gets(), it suffers from buffer overrun issues. Use fgets() instead.

Upvotes: 2

Vlad from Moscow
Vlad from Moscow

Reputation: 311048

What you are trying to do is not reversing a string. Neither string is reversed in your program. You are trying to copy one string in another string in the reverse order.

So the sring where you are going to copy the original string in the reverse order shall have enough space to store the copied characters.

However in your function

char* strrev(char *str)
{
  char *str1
  //...

pointer str1 was not initialized and does not point to an extent of memory of the appropriate size.

So your program has undefined behaviour.

Take also in account that function gets is unsafe and is not supported by the C standard any more. Use function fgets instead.

If your compiler supports Variable Length Arrays (VLA) then the program can look the following way

#include <stdio.h>
#include <string.h>

char * reverse_copy( char *s2, const char *s1 )
{
    size_t n = strlen( s1 );
    size_t i = 0;

    for ( ; i < n; i++ ) s2[i] = s1[n-i-1];

    s2[i] = '\0';

    return s2;
}

int main( void )
{
    char s1[1000];

    printf( "Please enter a String: " );
    fgets( s1, sizeof( s1 ), stdin );

    size_t n = strlen( s1 );

    if ( n != 0 && s1[n-1] == '\n' ) s1[n-1] = '\0';

    char s2[n + 1];

    puts( s1 );
    puts( reverse_copy( s2, s1 ) );

    return 0;
}

If to enter for example

Hello Asfakul Islam

then the output will look like

Please enter a String: Hello Asfakul Islam
Hello Asfakul Islam
malsI lukafsA olleH

Otherwise if your compiler does not support VLA(s) you need to dynamically allocate an array of the appropriate size. In this case the program can look for example the following way

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

char * reverse_copy( char *s2, const char *s1 )
{
    size_t n = strlen( s1 );
    size_t i = 0;

    for ( ; i < n; i++ ) s2[i] = s1[n-i-1];

    s2[i] = '\0';

    return s2;
}

int main( void )
{
    char s1[1000];

    printf( "Please enter a String: " );
    fgets( s1, sizeof( s1 ), stdin );

    size_t n = strlen( s1 );

    if ( n != 0 && s1[n-1] == '\n' ) s1[n-1] = '\0';

    char *s2 = malloc( ( n + 1 ) * sizeof( char ) );

    puts( s1 );
    puts( reverse_copy( s2, s1 ) );

    free( s2 );

    return 0;
}

Upvotes: 0

Haris
Haris

Reputation: 12272

In your function, you did not initialize c

int i,length,c;

and are using it inside the for loop

*(str1+c) = *(str+i);

Plus other problems are there..

1) str1 inside the function is not allocated memory.

2) This loop will never get executed, as the condition in for (i=length-1;i<=0;i--) is never true (unless string is 0 or 1 character long).

3) Do not use gets(), it is deprecated. Instead use fgets()

Upvotes: 0

Giorgi Moniava
Giorgi Moniava

Reputation: 28674

  for (i=length-1;i<=0;i--)

This will never (unless string is 0 or 1 character long) run due to i<=0, should be i>=0 probably.

Also in general you need to make pointer point to some valid memory in order to be able to dereference it. In your case you should probably use malloc for allocating sufficient number of bytes, and assign its result to str1. Then you can write to it as you are doing.

Upvotes: 2

MikeCAT
MikeCAT

Reputation: 75062

  1. You don't initialize str1 in strrev()
  2. If you enter string whose length is 2 characters or more, i<=0 is false and the block inside the for loop won't be executed.
  3. *(str1+c) ='\0'; will cause the crash because the value of str1 is indeterminate and you have few chance that str1 points some valid place.

UPD: c in strrev() is also uninitialized, and it will cause some trouble.

Upvotes: 0

Related Questions