C: Problem in comparing two strings in a function

Question

Good morning everyone, I have to simulate the operation of the strstr() function with a function written by me.

In the code I slide the original string in a temporary string and then the comparison with the string to look for, if they are equal it should return 1.

But even if the strings are equal and of the same length the code never enters the if cycle and therefore never returns 1.

My code:

int *strstr_new(char *s7, char *s8) {

int length_s7 = strlen(s7);
int length_s8 = strlen(s8);
char search_string[length_s8];
printf("%d\n", length_s8);

for(int i=0; i<length_s7; i++) {
    for(int j=0; j<length_s8; j++) {
        search_string[j] = s7[i+j];
        search_string[j+1] = '\0';
    }
    printf("%s\n", s8);
    printf("%s\n", search_string);
    printf("%d\n", length_s8);
    printf("%d\n", strlen(search_string));
    //search_string[length_s8+1] = '\0';
    if(search_string == s8) {
        return(1);
    }
}
if(search_string != s8) {
    return(NULL);
}}

Does someone have an idea of where I'm wrong?

Thanks!

Start by looking at your [compiler warnings](https://stackoverflow.com/questions/57842756). Then ask yourself this question: if ny function returns a *pointer*, what should it *point at*? — n. m. could be an AI, Oct 04 '20 at 09:25
Next, write this on the classroom chalkboard 100 times: *A string is a null terminated character array. I shall always provide an extra character for the null terminator.* — n. m. could be an AI, Oct 04 '20 at 09:28
Next, fire up the debugger. Or use a little paper-and-pencil debugger. You have `s7[i+j]` What's the maximum value of `i+j` again? — n. m. could be an AI, Oct 04 '20 at 09:32
Next, read about [`strcmp`](https://en.cppreference.com/w/c/string/byte/strcmp). Why do you think this function is needed? Where could you have used it in your code? — n. m. could be an AI, Oct 04 '20 at 09:33
When you have fixed all of the above problems, test your function with inputs of 2 million characters each. — n. m. could be an AI, Oct 04 '20 at 09:39

score 1 · Answer 1 · answered Oct 04 '20 at 09:33

1

The biggest problem in your code is comparing strings with == operator. Both search_string and s8 are char pointers, which means you're comparing addresses of different variables, obviously to return False. Try adding another for loop to compare each char in search_string to the corresponding char in s8 (using the dereferencing operator *).

answered Oct 04 '20 at 09:33

Eyal Waserman

29
2

yes now i create a function that works like strcmp()! – Cb95 Oct 04 '20 at 10:15

anastaciu · Answer 2 · 2020-10-04T10:58:32.110

Your string comparisons won't work because you are comparing the addresses of those strings instead of the strings themselves, you'd what to use something like strcmp or memcmp to compare two strings.
Your return type is also not compatible with the return you have particularly if the strings match. I'd return 1 if the string is found and 0 if it's not, for that you'd need to change the return type to int only.
The second string comparison is unneeded, you already test the existance of the substring inside the loop so you just need to return 0 if the loop finds it's way to the end.
Lastly the temporary string is too short and will allow for access outside its bounds, inside the loop.

e.g. if length_s8 is 4 will write to search_string[4], 5th index, out the bounds of the array.

int strstr_new(char *s7, char *s8) //return 1 for found, 0 for not found
{
    int length_s7 = strlen(s7);
    int length_s8 = strlen(s8);   
    char search_string[length_s8 + 1];//you'd want to avoid buffer overflow

    for (int i = 0; i < length_s7; i++)
    {
        for (int j = 0; j < length_s8; j++)
        {
            search_string[j] = s7[i + j];
            search_string[j + 1] = '\0';
        }

        if (!strcmp(search_string, s8))
        {
            return 1; //if the string is found return 1 immediately
        }
    }
    return 0; //if it reaches this point, no match was found
}

A couple of tests:

printf("%d\n", strstr_new("this is my string", "this i"));
printf("%d\n", strstr_new("this is my string", "ringo"));
printf("%d\n", strstr_new("this is my string", "ring"));
printf("%d\n", strstr_new("this is my strin", "ths"));

Output:

Vlad from Moscow · Accepted Answer · 2020-10-04T11:29:17.310

The function declaration

int *strstr_new(char *s7, char *s8);

looks very strange.

For example why is the return type int *? Why are function parameters named s7 and s8 instead of for example s1 and s2? Why are not the function parameters qualified with const?

Creating a variable length array within the function is inefficient and redundant and can lead to stack exhaustion.

char search_string[length_s8];

This loops

for(int j=0; j<length_s8; j++) {
    search_string[j] = s7[i+j];
    search_string[j+1] = '\0';
}

invokes undefined behavior because this statement

search_string[j+1] = '\0';

writes beyond the array when j is equal to length_s8 - 1.

In this statement

if(search_string == s8) {

there are compared two pointers and it is evident that they are unequal because they point to different arrays.

Without using standard C functions except the function strlen (that could be also defined explicitly) the function can be declared and defined the following way

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

char * strstr_new( const char *s1, const char *s2 )
{
    char *p = NULL;
    
    size_t n1 = strlen( s1 );
    size_t n2 = strlen( s2 );
    
    if ( !( n1 < n2 ) )
    {
        for ( size_t i = 0, n = n1 - n2 + 1; p == NULL && i < n; i++ )
        {
            size_t j = 0;
        
            while ( j < n2 && s1[i + j] == s2[j] ) ++j;
        
            if ( j == n2 ) p = ( char * )( s1 + i );
        }
    }
    
    return p;
}

int main( void ) 
{
    const char *s1 = "strstr_new";
    const char *s2 = "str";
    
    for ( const char *p = s1; ( p  = strstr_new( p, s2 ) ) != NULL; ++p )
    {
        puts( p );
    }
}

The program output is

strstr_new
str_new

If you are allowed to use standard string functions along with strlen then the loop within the function strstr_new can be simplified the following way

for ( size_t i = 0, n = n1 - n2 + 1; p == NULL && i < n; i++ )
{
    if ( memcmp( s1 + i, s2, n2 ) == 0 ) p = ( char * )( s1 + i );
}

C: Problem in comparing two strings in a function

3 Answers3