0

I am currently recoding srtcat() from the standard C library and I have set up some checks to avoid overlap problems. The problem is that my program still enters the error handling.

Here is the code:

char *my_strcat(char *restrict dest, const char *restrict src)
{
    size_t dest_len = 0, src_len = 0;
    char *p = dest;
    src_len = my_strlen(src);
    if (!dest || !src)
        return NULL;
    dest_len = my_strlen(dest);
    if (src >= dest && src < dest + dest_len) {
        return NULL;
    }
    if (dest >= src && dest < src + src_len) {
        return NULL;
    }
    while (*p != '\0') p++, dest_len++;
    if (dest_len + src_len + 1 > sizeof(dest))
        return NULL;
    p = dest + dest_len;
    while (*src != '\0')
        *p++ = *src++;
    *p = '\0';
    return dest;
}
size_t my_strlen(const char *s)
{
    size_t count = 0;
    if (s != NULL) {
        while (*s != 0) {
            count++;
            s++;
        }
    }
    return count;
}

I tested this way :

int main(int argc, char **argv)
{
    const char *src = "Hello";
    char dest[100] = " world!";
    char *test = my_strcat(dest, src);
    printf("Src : %s Dest : %s\n", src, dest);
    printf("Return adress : %p, Value : %s\n", test, test);
    return 0;
}

According to gdb :

if (src >= dest && src < dest + dest_len) 
1: dest = 0x7fffffffda70 " world!"
2: src = 0x555555557004 "Hello"
3: dest_len = 0
4: src_len = 5

Output

Src : Hello Dest :  world!
Return adress : (nil), Value : (null)

Do you see the problem?

Update

Following your suggestions I have modified the code like this:

char *my_strcat(char *restrict dest, const char *restrict src, size_t d_size)
{
    size_t dest_len = 0, src_len = 0;
    char *p = dest;
    if (!dest || !src)
        return NULL;
    src_len = my_strlen(src);
    dest_len = my_strlen(dest);
    if (src >= dest && src < dest + dest_len) {
        return NULL;
    }
    if (dest >= src && dest < src + src_len) {
        return NULL;
    }
    while (*p != '\0') p++, dest_len++;
    if (dest_len + src_len + 1 > d_size)
        return NULL;
    p = dest + dest_len;
    while (*src != '\0')
        *p++ = *src++;
    *p = '\0';
    return dest;
}

And in the main : char *test = my_strcat(dest, src, sizeof(dest));

But it still doesn't work :

Src : Hello Dest :  world!
Return adress : 0x7fff74bc5650, Value :  world!
  • 1
    `sizeof(dest)` in the function measures the size of the pointer. The function must be told by the caller the size of the destination array (in a 3rd parameter). The caller can _see_ the array and `sizeof` makes sense (as long as the elements are only 1 byte.) The function, as is, only receives the address of the first byte; nothing more... – Fe2O3 Dec 28 '22 at 01:31
  • Yes but I want to keep the same prototype as strcat so is there any other way? –  Dec 28 '22 at 01:34
  • 1
    In a word, no. This is why `strcat()` can be dangerous if used indiscriminately. When the function only gets pointers (addresses), it's up to the caller to make sure there's room in the destination array. (C's strings are null-terminated. You could, if you can, define some other "sentinel" to mark the absolute end of `dest`, but this would be considered "hacky" and would make one more special byte value that cannot appear as ordinary data. – Fe2O3 Dec 28 '22 at 01:39
  • The check `if (!dest || !src)` should be place **before** `src_len = my_strlen(src);` – Pablo Dec 28 '22 at 01:40
  • Okay, I modified it but it still doesn't work, the only difference is that now I have : `Return adress : 0x7ffc573e3db0, Value : world!` –  Dec 28 '22 at 01:46
  • If you're changing code on the fly (trying to fix things) the best way to make this question useful would be to edit the question APPENDING (minimally) what you hope might be the improved code as an extension of the original question. (Don't modify code that has been read by others already. This would/could invalidate responses already entered.) – Fe2O3 Dec 28 '22 at 01:51
  • Just noticed! In `main()` you're printing the returned 'pointer' from the function... Printing the %s string at NULL is going to give more headaches than you want... – Fe2O3 Dec 28 '22 at 01:56
  • No segfault for this time haha ! –  Dec 28 '22 at 01:58

2 Answers2

2

Having tried to guide toward understanding this problem, it seems best to present what should be working code (for study.) Sometimes too many words merely muddle the situation:

char *my_strcat(char *restrict dest, const char *restrict src, size_t d_size) {
    if( !dest || !src )
        return NULL;

    size_t src_len = strlen( src );
    size_t dest_len = strlen( dest );

    if( dest_len + src_len + 1 > d_size )
        return NULL;

    char *p = dest + dest_len;
    while( (*p++ = *src++ ) != '\0' )
        ;

    return dest;
}

int main() {
    const char *src = "Hello";
    char dest[100] = " world!";
    printf("Src : %s Dest : %s\n", src, dest);

    char *test = my_strcat( dest, src, sizeof dest );

    if( test )
        printf("Value : %s\n", test );

    return 0;
}

Now, one can experiment by shrinking the size of dest to something larger than " world!" but smaller than " world!Hello"... Perhaps 9 bytes???

And, now that the concatenation should be working (into a big enough buffer), adding the code to ensure there is no overlap of the actual character arrays. Known is the size of dest, and the length of src is measured.

Fe2O3
  • 6,077
  • 2
  • 4
  • 20
0
dest_len + src_len + 1 > sizeof(dest)

sizeof(dest) is the size of the pointer sizeof(char*). If you want to check if the dest will be overflowed, you have to pass the size as an argument. See strlcpy or strncpy.

src >= dest

Note that comparing pointers that do not point to the same array is technically invalid. To bring some breeze of validity, you can do (uintptr_t)stc >= (uintptr_t)dest. How does pointer comparison work in C? Is it ok to compare pointers that don't point to the same array? Is comparing two pointers with < undefined behavior if they are both cast to an integer type? Why does comparing pointers with undefined behavior still give correct results? etc.

KamilCuk
  • 120,984
  • 8
  • 59
  • 111
  • I modified it like you told me but it still doesn't work. Just that now the function returns a valid pointer to `dest` remains unchanged. –  Dec 28 '22 at 01:51