Can this function be any safer ? Looking for tips and your thoughts !

Question

this is somewhat of an odd question.

I wrote a C function. Its 'like' strchr / strrchr. It's supposed to look for a character in a c-string, but going backwards, and return a pointer to it. As c strings are not "null initiated", it also takes a third parameter 'count', indicating the number of chars it should look backwards.

/*
*s: Position from where to start looking for the desired character.
*c: Character to look for.
*count: Amount of tests to be done
*
* Returns NULL if c is not in (s-count,s)
* Returns a pointer to the occurrence of c in s.
*/
char* b_strchr(const char* s,int c,size_t count){

    while (count-->0){

        if (*s==c) return s;
        s--;
     }
     return NULL;
}

I have done some testing on it, but Do you see any flaws in it? Security issues or so? Any enhancements? Could it be improved? And more important: Is this a bad idea?

Some usage.

    char* string = "1234567890";

    printf("c: %c\n",*b_strchr(string+9,'5',10));//prints 5

    printf("c: %c\n",*b_strchr(string+6,'1',7));//prints 1

EDIT: New interface, some changes.

/*
* from:  Pointer to character where to start going back.
* begin: Pointer to characther where search will end.
*
* Returns NULL if c is not between [begin,from]
* Otherwise, returns pointer to c.
*/
char* b_strchr(const char* begin,int c,const char* from){


    while (begin<=from){

        if (*from==c) return from;
        from--;
     }
     return NULL;
}

Answer 1

It's better with the edit, but the interface is still surprising. I'd put the begin parameter (the haystack being searched) as the first parameter, the c parameter (the needle being searched for) second, and the from parameter (start position of the search) third. That order seems to be idiomatic across a fairly large set of APIs.

Answer 2

The code has an esoteric interface - pass in a pointer to the last character of the string and the length of the string. That will lead to problems using it.

(Alternatively, the code has a bug - you should add count to s before the loop.)

Answer 3

If begin is from, the current code will always return begin, which is not what you want. The code after the loop can just be return NULL. And instead of begin != from in the loop condition, I would use begin < from otherwise you will pointer arithmetic overflow when someone mixes up the order of the parameters.

Edit: on second thought since you want [begin, from] inclusive it should be begin <= from

Answer 4

I wrote a C function. Its 'like' strchr / strrchr.

You've attempted to reinvent strrchr(), so it's not like strchr().

Do you see any flaws in it?

Yes. Several. :-(

Since b_strchr() can return NULL, you shouldn't put it directly into the printf() statement. Deferencing NULL usually results in a segfault.

You may be better off with your favourite variation of ...

char *result;

result = b_strchr(string + 9, 'a', 10));
if (result == NULL)
{
    printf("c: NULL\n");
}
else
{
    printf("c: %c\n", *result);
}

Also, when

(count >= length of the input string) and the character is not found

you're going to get unpredicable results because s is no longer pointing to a character in the string — s is pointing to memory before the beginning of the string. As an example, try

result = b_strchr(string + 9, 'a', 11));
if (result == NULL)
{
    printf("c: NULL\n");
}
else
{
    printf("c: %c\n", *result);
}

and see what happens.

Expand your use test cases to include conditions outside of what you know will work successfully. Ask someone else to help you design test cases that will really test your code.

And more important: Is this a bad idea?

As a learning exercise, absolutely not.

However, in this case, for production code you'd be better off sticking to the standard strrchr().