Unexpected return from string comparison function

[Nessie]

Hi all

The string_in function below is intended to take two string pointers as arguments and if the first is contained in the second, return the address at which the contained string begins.

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

#define TEST "This ain't a drill"

char * string_in (char * str1, char * str2) ;

int main (void)
{
	char instr [10] ;
	char * res ;

	printf ("Please enter the string to look for:"
			"(EOF to quit)\n") ;
	while ((gets(instr)) != NULL)
	{
		if ((res = string_in (instr, TEST)) != NULL)
			printf ("%s is located %d characters into TEST.\n",
					instr, res - TEST) ;
		else
			printf ("Nope.\n") ;
	}
	printf ("Thanks for playing...\n") ;
}

char * string_in (char * str1, char * str2)
{
	int i, j ;
	char * res ;

	for (i = 0; i < strlen (str2); i++)
	{
		if (str2[i] == str1[0])
		{
			res = str2 + i ;
			for (j = 0; j < strlen (str1); j++)
			{
				if (str2[i + j] != str1[j])
				{
					res = NULL ;
					break ;
				}
			}
		}
	}
	return res ;
}

I get the following output when testing it:

geoff@geoff-laptop:~/prog/C$ gcc string_in.c -o string_in
/tmp/ccATd9U0.o: In function `main':
string_in.c:(.text+0x80): warning: the `gets' function is dangerous and should not be used.
geoff@geoff-laptop:~/prog/C$ ./string_in
Please enter the string to look for:(EOF to quit)
This
This is located 0 characters into TEST.
ain't
Nope.
ain
Nope.
ai
Nope.
a
a is located 11 characters into TEST.
Thanks for playing...

I figure that the reason why "ain't" is not picked up here must have something to do with the apostrophe, but for the life of me I can't see what :confused: I also tested "This ain't" and got a positive result. Could it have something to do with using gets()?

[Ancient Dragon]

never ever for any reason use gets(). Here's why.

>>Could it have something to do with using gets()?
Yes. instr can only hold 9 characters plus the null terminator and you are trying to stuff more than that into it. gets() is scribbling the overflow all over your program's memory.

[Nessie]

Thanks for that, I've replaced it with a call to fgets() ;

int main (void)
{
	char instr [10] ;
	char dummy [50] ;
	char * find ;
	char * res ;

	printf ("Please enter the string to look for:"
			"(EOF to quit)\n") ;
	while ((fgets(instr, 10, stdin)) != NULL)
	{
	    find = strchr (instr, '\n') ;
	    if (find)
	        *find = '\0' ;
	    else
	    {
	        instr[0] = 0 ;
	        do
	        {
	            fgets (dummy, 50, stdin) ;
	            strcat (instr, dummy) ;
	        } while (dummy[strlen(dummy) - 1] != '\n') ;
	    }
...

However, the output is still the same (of course, minus the compiler warning me about gets()):

geoff@geoff-laptop:~/prog/C$ gcc string_in.c -o string_in
geoff@geoff-laptop:~/prog/C$ ./string_in
Please enter the string to look for:(EOF to quit)
This
This is located 0 characters into TEST.
ain't
Nope.
ain
Nope.
a
a is located 11 characters into TEST.
drill
drill is located 13 characters into TEST.
Thanks for playing...

Any ideas?

[Ancient Dragon]

two problems:
1) line 29: initialize the pointer to NULL because if the loop beginning on line 31 never finds the first character of the string then the function needs to return a valid pointer -- NULL.

2) Add a break just after the end of the loop on line 43 because at that point it has found the string so no point in looking any further.

[Nessie]

Thanks again :). After making both those changes, I realised that there was still an issue, as the loop should only break if the entire string was actually found, so I put in a flag:

char * string_in (const char * str1, char * str2)
{
	int i, j ;
	char * res = NULL ;
	bool found = false ;

	for (i = 0; i < strlen (str2); i++)
	{
		if (str2[i] == str1[0])
        {
            res = str2 + i ;
            found = true ;
            for (j = 0; j < strlen (str1); j++)
            {
                if (str2[i + j] != str1[j])
                {
                    res = NULL ;
                    found = false ;
                    break ;
                }
            }
            if (found == true)
                break ;
        }
	}
	return res ;
}

This now works exactly as intended.

However, along the way I also decided to try using strncmp, so I modified the function as follows:

char * string_in (char * str1, char * str2)
{
	int i ;
	char * res = NULL ;

	for (i = 0; i < strlen (str2); i++)
	{
		if (strncmp (str2[i], str1, strlen(str1)) == 0)
		{
			res = str2 + i ;
			break ;
		}
	}
	return res ;
}

When I try to compile, I get the following response:

geoff@geoff-laptop:~/prog/C$ gcc string_in1.c -o string_in1
string_in1.c: In function ‘string_in’:
string_in1.c:50: warning: passing argument 1 of ‘strncmp’ makes pointer from integer without a cast

If any light can be shed on this I'd appreciate it...what's wrong with the arguments I'm passing to strncmp()?

[Ancient Dragon]

The first argument to strncmp() is a const char* pointer, but you are just passing a single character.

Suggest you change line 8 like this: if (strncmp (&str2[i], str1, strlen(str1)) == 0) Actually there is an easier way to do this using strstr().

[Nessie]

Of course! Thanks again, again. :)

Actually there is an easier way to do this using strstr().

Yeah, I think the point of the exercise may have been to construct a function which did the same thing as strstr(). It may not have been the most meaningful exercise but I've learnt a lot from it - thanks for your help.