Disappearing Strings in C

adymroxx · 01-31-2007, 09:30 AM

I'm working on a simple shell program as part of a class project and I've hit a dead end. I'm new to C programming, so I could be missing something pretty obvious here, but I have pinpointed it to a certain chunk of my code. In the following snippet, the first for loop prints off all of the paths in the user's ENV path variable. In the second loop, it does the same only this time it tacks on a "/" character.

---snippet---
char *lookupPath(char **argv, char **dir) {

char *temp;

/* Not sure if the following line is necessary... */
temp = (char *) malloc(MAX_PATH_LEN * 2);
/* Print user's path variables */
for (i = 0; i < 8; i++) printf("%d= %s\n", i, dir[i]);
/* Look in PATH directories */
/* Use access() to see if the file is in a dir */
for (i = 1; i < MAX_PATHS; i++) {
temp = NULL;
if (dir[i] == NULL) continue;
temp = dir[i];
strncat(dir[i], "/", MAX_PATH_LEN);
printf("Looking %d in %s\n", i, temp);
.
.
.
}
---end snippet---

This is what prints:
---script---
$ ./shell
# ls
0= (null)
1= /usr/kerberos/bin
2= /usr/lib/ccache
3= /usr/local/bin
4= /usr/bin
5= /bin
6= /usr/X11R6/bin
7= /home/adam/bin
Looking 1 in /usr/kerberos/bin/
Looking 2 in /
Looking 3 in /usr/local/bin/
Looking 4 in /
Looking 5 in /bin/
Looking 6 in /
Looking 7 in /home/adam/bin/
---end script---

For reasons unbeknowest to me, only the odd numbered paths are printing.
One second dir[i] contains a path, the next moment it's null. Where are
my paths going??

Thanks in advance!

kaz2100 · 01-31-2007, 10:31 AM

Hi,

#1, Your first loop and second loop, initial value and loop condition are not same.
#2. It seems that you want to add "/" at the end of dir[i], (issue hiding here, potentially HUGE.) and screwing up succeeding member.

Your penguin will do what you ask...

Adding "/" to dir[n] is DANGEROUS, unless you know you have allocated free space after dir[n], which

Code:

char *lookupPath(char **argv, char **dir)

implies "NO, you do not". You may be overwriting first byte of next member or something else. (it depends on complier implementation.) C compiler usually does not detect it, as it trusts you. This may end up with run time error "Segmentation Fault"

Happy Penguins!

adymroxx · 01-31-2007, 10:44 AM

Quote:

Originally Posted by kaz2100

Hi,

#1, Your first loop and second loop, initial value and loop condition are not same.
#2. It seems that you want to add "/" at the end of dir[i], (another issue hiding here, potentially HUGE), and printing out temp.

These two issues are a kind of forgettable, tiny and benign. Your penguin will do what you ask...

Adding "/" to dir[n] is DANGEROUS, unless you know you have allocated free space after dir[n], which

Code:

char *lookupPath(char **argv, char **dir)

implies "NO, you do not". You may be overwriting first byte of next member or something else. (it depends on complier implementation.) C compiler usually does not detect it, as it trusts you. This may end up with run time error "Segmentation Fault"

Happy Penguins!

Shouldn't this line

Code:

temp = (char *) malloc(MAX_PATH_LEN * 2);

give enough room to add "/" to the previous string? Keep in mind that I'm pretty new to C... should I call malloc once in each loop?

Thanks again

graemef · 01-31-2007, 10:49 AM

I think that your problem lies with the adding the / to dir rather than another variable you so carefully set up and forgot to use.
dir is an array of strings and the memory will look as follows:

Code:

0/usr/kerberos/bin0/usr/lib/ccache0/usr/local/bin0/usr/bin0/bin0/usr/X11R6/bin0/home/adam/bin
^^dir[1]           ^               ^              ^        ^    ^              ^
dir[0]             dir[2]          dir[3]         dir[4]   dir[5]dir[6]        dir[7]

Where 0 is teh NULL character. Now when you add a slash to the end of dir[1] your new memory will be as follows:

Code:

0/usr/kerberos/bin/0usr/lib/ccache0/usr/local/bin0/usr/bin0/bin0/usr/X11R6/bin0/home/adam/bin
^^dir[1]           ^               ^              ^        ^    ^              ^
dir[0]             dir[2]          dir[3]         dir[4]   dir[5]dir[6]        dir[7]

And so dir[2] points to a NULL character.

eddiebaby1023 · 01-31-2007, 11:02 AM

I think dir is an array of pointers to strings. We don't see all the code in the second loop - are you incrementing i within the loop as well as in the for line?

graemef · 01-31-2007, 11:10 AM

dir is essentially argc

adymroxx · 01-31-2007, 11:17 AM

The entire function is as follows:

Code:

char *lookupPath(char **argv, char **dir) {
	/* This function searched the directories identified by the dir
	 * argument to see if argv[0] (the file name) appears there.
	 * Allocate a new string, place the full path name in it, then
	 * return a string. */
	char *result;
	char *temp;
	int  i;

	/* Check to seee if file name is already an absolute path */
	if (*argv[0] == '/') {
		result = argv[0];
		return result;
	}

	temp = (char *) malloc(MAX_PATH_LEN * 2);

	for (i = 0; i < MAX_PATHS; i++) printf("%d= %s\n", i, dir[i]);
	/* Look in PATH directories */
	/* Use access() to see if the file is in a dir */
	for (i = 0; i < MAX_PATHS; i++) {
		temp = dir[i];
		if (dir[i] == NULL) continue;
		temp = dir[i];
		strncat(temp, "/", MAX_PATH_LEN);
		//strncat(temp, argv[0], MAX_PATH_LEN);
		printf("Looking %d in %s\n", i, temp2);
		//if (access(dir[i], X_OK) == 0) {
		//	result = temp;
		//	return result;
		//}
	}
	/* File name not found in any path variable */
	fprintf(stderr, "%s: command not found\n", argv[0]);
	return NULL;
}

I've changed the strncat to add to the temp variable rather than dir[i] and the problem persists.

graemef · 01-31-2007, 11:23 AM

Okay, that is because you do temp = dir[i], so temp points to where dir[i] points to, you need to copy dir[i] into temp then you should be alright.

adymroxx · 01-31-2007, 11:34 AM

Quote:

Originally Posted by graemef

Okay, that is because you do temp = dir[i], so temp points to where dir[i] points to, you need to copy dir[i] into temp then you should be alright.

That seems to work!

Within the second for loop I now have

Code:

	for (i = 0; i < MAX_PATHS; i++) {
		if (dir[i] == NULL) continue;
		temp = (char *) malloc(MAX_PATH_LEN * 10);
		strcpy(temp, dir[i]);
		strncat(temp, "/", MAX_PATH_LEN);
		strncat(temp, argv[0], MAX_PATH_LEN);
		printf("Looking %d in %s\n", i, temp);

I know MAX_PATH_LEN * 10 is too big, but I'll round it down to the correct size later.

Thank you all for your help!

graemef · 01-31-2007, 11:39 AM

Great stuff

PTrenholme · 01-31-2007, 11:39 AM

<curmudgeon>
It's problems like this -- memory allocation and pointers -- that made me decide, many years ago when I was a programming group manager, that the first programming course any programmer should list in their resume had to be an assembly language course. Anyone without that qualification had to work really hard to convince me to hire them.

I sometimes, but rarely, made exceptions for programmers with bcpl experience for non-trivial applications.
</curmudgeon>

kaz2100 · 01-31-2007, 01:51 PM

It seems that adymroxx learned step ZERO in C programming. Now, adymroxx cannot use "I am new in C" excuse any longer.

graemef · 01-31-2007, 09:44 PM

Quote:

Originally Posted by PTrenholme

<curmudgeon>Anyone without that qualification had to work really hard to convince me to hire them.

Guess I wouldn't of got hired then...