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, Oct 31 '13 at 00: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, Oct 31 '13 at 00:51

Charlie Burns · Answer 1 · 2013-10-31T14:44:13.537

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 · Answer 2 · 2013-10-31T10:25:09.523

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 '13 at 10:25

answered Oct 31 '13 at 00:55

Kninnug

7,992
1
30
42

@Mike gave a very helpful hint in the question-comments. :) – Kninnug Oct 31 '13 at 01:00
Yeah, that's pretty much the solution I was thinking of too, until I saw he was passing in string constants. =) – mpontillo Oct 31 '13 at 01:01
One simple `char str[] = "...";` should fix that in this case. – Kninnug Oct 31 '13 at 01:02
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. – mpontillo Oct 31 '13 at 01:03
Maybe, remember that `memmove` may be very heavily optimized [citation needed], so it's probably equally fast. – Kninnug Oct 31 '13 at 01:05
Actually this implementation doesn't work at all. `strlen()` returns the string that was passed to it. So it'll return `s + pos` instead of properly returning `s`. – mpontillo Oct 31 '13 at 01:45
`strlen` returns the length of the string that was passed to it, not the string itself. Though I am doubtful about the size parameter to `memmove` should that be `strlen(s) - n - pos + 1`? (Tests with Dr. Memory & Valgrind show no problems with the current implementation) – Kninnug Oct 31 '13 at 01:58
`memmove()` always returns the first parameter passed to it. You'll get back `s + pos` (the thing you're copying) not `s` (the entire string). If you pass this function `("abc", 1, 1)`, you should get a pointer to a string `"ac"`, but your implementation returns `"c"`. – mpontillo Oct 31 '13 at 03:21

mpontillo · Answer 3 · 2013-10-31T00:56:03.140

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;
}

C String Pointer Function strdel

3 Answers3

Linked