C String Pointer Function strdel

Question

Can someone please explain me why I get "Segmentation fault..." and how to fix it on this bit of code?

#include<stdio.h>

int str_length(char *s) {
    int length = 0, i;
    for(i = 0; *s; i++) {
        s++;
    }
    return i;
}

char *strdel(char *s, int pos, int n) {
    int i;
    char *p, str[] = "";
    p = str;
    for(i = 0; i < str_length(s) - n + 1; i++)  {
        if(i >= pos) {
            *(p + i) = *(s + i + n);
        }
        else {
            *(p + i) = *(s + i);
        }
    }
    s = str;
    return s;
}

int main() {
    char *str = "abcdef";
    printf("str_lengh: %d\n", str_length(str));
    printf("strdel: %s\n", strdel(str, 1, 2));
    return 0;
}

And I get this output:

str_lengh: 6
strdel: adef
Segmentation fault (core dumped)

Also, is there a better way to create a function: char *strdel(char *s, int pos, int n); that deletes the n characters from position pos than the one I did?

To address your second question, I would have used memmove(). You've got a loop that calculates the length of the string every iteration, which isn't going to be efficient. — mpontillo
– mpontillo, Commented Oct 31, 2013 at 0:38
char * strdel(char * s, int pos, int n){ memmove(s + pos, s + pos + n, strlen(s) - n + 1); return s; } should so it, though it doesn't copy. Nor does it do any bounds checking. — Kninnug
– Kninnug, Commented Oct 31, 2013 at 0:51

Charlie Burns · Accepted Answer · 2013-10-31 14:44:13Z

I think you are writing all over the stack here...

char *strdel(char *s, int pos, int n) {
    int i;
    char *p, str[] = "";
    p = str; // p points to str which is "" and is on the stack with length 0.
    for(i = 0; i < str_length(s) - n + 1; i++)  {
        if(i >= pos) {
            *(p + i) = *(s + i + n); // now you are writing onto the stack past p
        }
        else {
            *(p + i) = *(s + i);// now you are writing onto the stack past p
        }
    }
    s = str; // now s points to space on stack
    return s; // now you return a pointer to the stack which is about to disapear 
}

Whenever you write past p, which is often, you are running into Undefined Behavior. UB You are writing into space which has not been allocated on the heap or on the stack.

You can write a version of strdel that works only on s. Something like this if I understand strdel right: (roughly, not tested!, needs bounds checking on pos and n )

char *strdel(char *s, int pos, int n) {
    char *dst = s + pos, *src = s + pos + n;
    while(*src) {
        *dst++ = *src++;
    }
    *dst = 0;
    return s;
}

Kninnug · Accepted Answer · 2013-10-31 10:25:09Z

2

I'll throw in my solution for the second part as well. Here's my strdel

char * strdel(char * s, int pos, int n){ 
    memmove(s + pos, s + pos + n, strlen(s) - pos - n + 1); 
    return s;
}

It doesn't copy, it doesn't do bounds checking and the return-value is rather redundant (as it's equal to the input s). So all-in-all it's very standard-C-library-like.

Warning! Cannot be used for string-constants as it modifies s (hence no const char * s).

edited Oct 31, 2013 at 10:25

answered Oct 31, 2013 at 0:55

Kninnug

8,1121 gold badge34 silver badges45 bronze badges

8 Comments

Kninnug Over a year ago

@Mike gave a very helpful hint in the question-comments. :)

mpontillo Over a year ago

Yeah, that's pretty much the solution I was thinking of too, until I saw he was passing in string constants. =)

Kninnug Over a year ago

One simple char str[] = "..."; should fix that in this case.

mpontillo Over a year ago

Looping until you hit the '\0' might actually be more efficient since you wouldn't have to calculate the length of the string and then traverse it again.

Kninnug Over a year ago

Maybe, remember that memmove may be very heavily optimized [citation needed], so it's probably equally fast.

|

mpontillo · Accepted Answer · 2013-10-31 00:56:03Z

0

To address the second part of your question, I would have written it something like this (assuming you're going to be passing in string constants and therefore must make a copy of the incoming string):

/*
 * Returns a copy of the NUL terminated string at s, with the
 * portion n characters long starting at position pos removed.
 */
char* strdel(char* s, int pos, int n)
{
    int size = strlen(s);
    char* t = malloc(size - n);
    memcpy(t, s, pos);
    memmove(t + pos, s + pos + n, size - n + 1);
    return t;
}

edited Oct 31, 2013 at 0:56

answered Oct 31, 2013 at 0:47

mpontillo

14k7 gold badges66 silver badges93 bronze badges

Collectives™ on Stack Overflow

C String Pointer Function strdel

3 Answers 3

Comments

8 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

8 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related