Thread: I can't tell what is wrong with this code ARRR!

I have written a get_line function to read lines from a file. The file contains dictionary phrases and defintion file names separated by a ";". I have also written a read_ index function that reads form the file a line using get_line then splits the line up into it's needed parts and then inserts the 2 parts into a BST. For the last 2 weeks, I have not been able to get this to work on my school server, although it works fine on my home pc. Is there anyone that can help me spot the bug? I am totally exhausted and have tried over 15 different configurations to quell the problem with no success. Here is the code, I suspect the problem may lye in freeing the buffer after it is used...
Please HeLp Me!

Code:

char *get_line(FILE *file) 
{
	FILE *fp = file;
	
	char *buffer;
	int ch,  i=0;
	unsigned int buffer_size = BUFFER_SIZE, new_buffer_size;

	assert(buffer_size > 0);

	buffer = (char *) safe_malloc (buffer_size * sizeof (char));
	
	while ((ch = getc(fp)) != EOF && ch != '\n')
	{
		if (buffer_size == i) 
		{
			new_buffer_size = buffer_size * 2;
			assert(new_buffer_size >= buffer_size);
			buffer = (char *) safe_realloc (buffer, new_buffer_size * sizeof (char));
			buffer_size = new_buffer_size;
		}
		buffer[i] = ch;
		i++;
	}
	if (ch == '\n')			 
	{
		buffer[i] = '\0';
		return buffer; 		/*finish it off with a null byte*/ 
	}
	if ((ch == EOF) && (i >= 1))
	{
		buffer[i] = '\0';
		return buffer;
	}
	else
	{
		free (buffer);
		return NULL;
	}
}

Code:

bstree_t *read_index (char *file_name)
{
	FILE *fp;
	char *buffer, *phrase, *def_file_name;
	bstree_t *index_tree = NULL;
	size_t str_len, phrase_len, def_file_name_len;
	
	fp = safe_open (file_name, "r");
		
	while ((buffer = get_line(fp)) != NULL)
	{
		/*split the string at the point ";" into phrase and def_file_name then insert the value
		*/
		str_len = strlen (buffer);
		phrase_len = strcspn (buffer, ";");
		phrase = (char *) safe_malloc (phrase_len + 1 * sizeof (char));
		strncpy (phrase, buffer, phrase_len);
		phrase[phrase_len+1] = '\0';
	
		def_file_name_len = str_len - phrase_len - 1;
	
		def_file_name = (char *) safe_malloc ((def_file_name_len+1) * sizeof (char));
	
		strcpy (def_file_name, buffer + phrase_len + 1);
	
		def_file_name[def_file_name_len-1] = '\0';
		
		index_tree = bstree_insert (index_tree, phrase, def_file_name);

	}
	
   return index_tree;
}

[/code]

What happenes when you run this in the school. Do you get any runtime error. What compile do you use at school? Did you use this same input of what u used at home.

ssharish

There's one definite error I see in the code. You wind up doing this:

phrase = (char *) safe_malloc (phrase_len + 1 * sizeof (char));
phrase[phrase_len+1] = '\0';

You're writing one beyond the end of your allocated space. If you allocate n bytes, you can only write up to the n-1st byte. That is to say, if you do:

char *s = malloc(5);

You're only allowed to access s[0], s[1], s[2], s[3], and s[4] (assuming, of course, the malloc() succeeds).

I use gcc with the flags "-Wall -ansi -g -o" both at home and at school. School uses solaris, I use debian. Debugging with gdb shows different values on the school computer compared to my home computer. I.E, the right ones on my pc, the wrong ones on the school pc. For example at home I can read in the line : "DNCRI;def2034" and get "DNCRI" and "def2034", but when using the school ocmputers, a few values turn out wrong "@NCRI" etc. and cause a segmentation fault when reading the index into the BST. BTW my line endings are set at LF for unix.

cas are you saying that :

phrase = (char *) safe_malloc (phrase_len + 1 * sizeof (char));
phrase[phrase_len+1] = '\0';

should be :

phrase = (char *) safe_malloc (phrase_len + 1 * sizeof (char));
phrase[phrase_len] = '\0'; ?



I know I need one more for the null, but does this mean that phrase_len+1 updates phrase_len by one and thus only phrase_len alone needs to be used for string phrase?

Code:

def_file_name_len = str_len - ( phrase_len - 1 );

change that, some time you will end up with the negative value.

Code:

def_file_name[def_file_name_len-1] = '\0';

Wouldn't this need to be

Code:

def_file_name[def_file_name_len + 1] = '\0';

ssharish

thanks ssharish, will give your advice a shot

Originally Posted by qubit67

cas are you saying that :

phrase = (char *) safe_malloc (phrase_len + 1 * sizeof (char));
phrase[phrase_len+1] = '\0';

should be :

phrase = (char *) safe_malloc (phrase_len + 1 * sizeof (char));
phrase[phrase_len] = '\0'; ?



I know I need one more for the null, but does this mean that phrase_len+1 updates phrase_len by one and thus only phrase_len alone needs to be used for string phrase?

I assume if buffer points to "DNCRI;def2034" then you want phrase to point to (the new string) "DNCRI". If that's the case then yes, the substitution you mentioned is correct.

In this case, phrase_len would be 5, because "DNCRI" has a length of 5. So to make a string, you'd want to allocate 6 bytes, which is phrase_len + 1. Because strncpy() is a bizarre little function (as you are apparently aware, it doesn't always create a string), you need to add the null byte yourself. In this case, it's at location 5, or phrase[phrase_len]. Array members 0, 1, 2, 3, 4 are the characters you just copied over, so member 5 is where a null byte goes. As you can see, 0-5 is 6 elements, which is why you used phrase_len + 1 as the size to allocate.

ok thankyou, I get it now, the array starts at 0 and the length needs to be re-adjusted for this.

Still trying to get it right, the chages i have made have not solved my problem

#define BUFFER_SIZE 1

i should have included this in my post

As far as i can see from gdb :

read_index (file_name=0x8061a68 "index") at project_libs.v7.c:131
131 while ((buffer = get_line(fp)) != NULL)
(gdb)
get_line (file=0x80616c0) at project_libs.v7.c:48
48 FILE *fp = file;
(gdb)
51 int ch, i=0;
(gdb)
52 unsigned int buffer_size = BUFFER_SIZE, new_buffer_size;
(gdb)
54 assert(buffer_size > 0);
(gdb)
56 buffer = (char *) safe_malloc (buffer_size * sizeof (char));
(gdb)
safe_malloc (bytes=1) at project_libs.v7.c:38
38 if ((p = malloc (bytes)) == NULL)
(gdb)
43 return p;
(gdb)
44 }
(gdb)

buffer is reset to 1 every time the function is used,
Am i still at risk of overflow with this?

ok, so i think if i take out the guard of the new line char in the while loop, and put it into the body of the while loop, after the i++ in an if statement it will work for the new line.

But what about the EOF in the while guard?

I need it there for the end of file, the function should return null for the read index function if it is encountered on a new line as the first char.

If the EOF is encountered at the end of a string, I need to insert a '/0' in place of the EOF and return the buffer as opposed to a new line with just the EOF : return NULL.

How do I design for this?