Thread: malloc does not seem to return

I have a function that walks through a message string. The message string has a begin indicator, and an END indicator en a Separator to separate the key=value pairs in the string. It should return a linked list to struct's that each contain a key value pairs so it can be processes elsewhere in the program.

I will describe briefly what happens. If I call the function with a teststring, it processes a few key value pairs, then after processing two key value pairs it does not seem to return from the malloc call, that I have marked red and bold.

I am aware that this may not be detailed enough, but I can post additional code/information if needed. Maybe one of the giants here spots the mistake in this code already.

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include "messaging.h"

struct keyValuePair {
	int key;
	char *value;
	struct keyValuePair *next;

struct keyValuePair *parseMessageString(char *aMessageString) {

	//set walker to the first position of the first key of the message
	char *walker = aMessageString + strlen(BEGIN);
	char *end = strstr(aMessageString, END); //this ptr is set at the end of the messg
	char *sep; //

	char *key = malloc(MAXFIELD * sizeof(*key + 1));
	char *value;
	int i = 0; //index for key and value pointers
	int type;  //value for fieldtype

	struct keyValuePair *firstPair = NULL;
	struct keyValuePair *prevPair;
	struct keyValuePair *currPair;

	//walk through message string, convert keys into integer
	//and store key and value in the linked list firstPair
	while (walker != end) {

		//get key digits untill you encounter '='
		i = 0;
		while ((*walker != '=')) {
			key[i] = *walker;
			++walker;
			++i;
		}
		key[i] = '\0'; //key[] contains key now
		type = atoi(key);
printf("key= %i \n", type);
	
		//set sep to next separator or the end for next while loop
		sep = ((strstr(walker, SEP) == NULL) ? end : strstr(walker, SEP));

		//store the value
		walker++; //was on the = sign
		i = 0;
		value = malloc(MAXFIELD * sizeof(*value + 1));
		if (value == NULL) printf("\nMalloc issue\n");
		while (walker != sep)
			value[i++] = *(walker++);
		value[i] = '\0';//value[] contains now the value
printf("VAL value= %s\n", value);

		currPair = malloc(sizeof(currPair));
printf("namallod\n");
		if (currPair == NULL) printf("\nMalloc Error\n");
		currPair->key = type;
		currPair->value = value;
		currPair->next = NULL;


printf("CURR key= %i value= %s\n", type, value);
		//add cuurPair to the linked list
		if (firstPair == NULL) {
			firstPair = currPair;
			prevPair = currPair;
		}
		else {
			prevPair->next = currPair;
			prevPair = currPair;
		}
		if (sep != end)
			walker += strlen(SEP);
	}	
	return firstPair;
}

Code:

value = malloc(MAXFIELD * sizeof(*value + 1));

I am not sure what this line does! I would guess it is not what you want it to do.

Tim S.

I put this line in for the following reason.
At first, I had value declared just as a char pointer. What then happened was,t hat the pointer firstPair, who is supposed to point at the first record, changed the value for firstPair->value every time an struct was added to the linked list. I had malloc-ed value right under where I malloc 'key'. My reasoning was then that value was pointing at the same memory all the time, so changing 'value' would change the ->value members in all structs, which indeed seemed to be the case. So then I figured that I had to give value a new piece of memory for every record in the linked list.

Originally Posted by grumpy

I've only looked at your malloc() calls. All are broken, so I have not looked further.

I'll recreate each line here, with a comment showing what each is ACTUALLY equivalent to. Check and see if you are doing what you expect.

Ok, makes sense. Let me briefly explain my thinking behind every malloc and maybe you can correct any wrong thinking.

Code:

      char *key = malloc(MAXFIELD * sizeof(*key + 1));       /*   char *key = malloc(MAXFIELD * sizeof(int))   */

the key in every key=value pair is a string, which gets converted to an int. So my thinking was, I have to reserve the max length of this field. When i convert it to an integer it gets copied, so I can keep reusing this field.

Code:

     value = malloc(MAXFIELD * sizeof(*value + 1));           /*    value = malloc(MAXFIELD * sizeof(int))  */

The value field of the key=value pair does not get converted. If I malloc value only one time, it means that the ->value pointers in the struct keyValuePair, all point to the same memory segment which implies that they all point to the same value. This is way I allocate it every time. The size is that of the maximumlength of the fiels + 1 for the \0.

Code:

     currPair = malloc(sizeof(currPair));                             /*   currPair = malloc(sizeof(struct KeyValuePair *))   */

My reasoning: currPair points every time to the new record to be added. I need to reserve space for this every time I add a new entry to the linked list. The size is one entry (=key value pair) at a time.

Curious to have any flaws here pointed out...

Originally Posted by Salem

> char *key = malloc(MAXFIELD * sizeof(*key + 1));
The thing is, sizeof evaluates the type, not the expression. So sizeof(1) and sizeof(1000) are both the same, namely the size of an int.

You might want instead
char *key = malloc(MAXFIELD * ( sizeof(*key) + 1) );

Ok, I see what you mean. But the '+1" was merely intended as one extra byte for the '\0'. So I am only 1 off. (but Iguess this can be fatal). Anyway, I corrected it but it does not change anything.

Code:

struct keyValuePair {
	int key;
	char *value;
	struct keyValuePair *next;
};

struct keyValuePair *parseMessageString(char *aMessageString) {

	//set walker to the first position of the first key of the message
	char *walker = aMessageString + strlen(BEGIN);
	char *end = strstr(aMessageString, END); //this ptr is set at the end of the messg
	char *sep; //

	char *key = malloc((MAXFIELD * sizeof(*key)) + 1);
	char *value;
	int i = 0; //index for key and value pointers
	int type;  //value for fieldtype

	struct keyValuePair *firstPair = NULL;
	struct keyValuePair *prevPair;
	struct keyValuePair *currPair;

	//walk through message string, convert keys into integer
	//and store key and value in the linked list firstPair
	while (walker != end) {

		//get key digits untill you encounter '='
		i = 0;
		while ((*walker != '=')) {
			key[i] = *walker;
			++walker;
			++i;
		}
		key[i] = '\0'; //key[] contains key now
		type = atoi(key);
printf("key= %i \n", type);
	
		//set sep to next separator or the end for next while loop
		sep = ((strstr(walker, SEP) == NULL) ? end : strstr(walker, SEP));

		//store the value
		walker++; //was on the = sign
		i = 0;
		value = malloc((MAXFIELD * sizeof(*value)) + 1);
		if (value == NULL) printf("\nMalloc issue\n");
		while (walker != sep)
			value[i++] = *(walker++);
		value[i] = '\0';//value[] contains now the value
printf("VAL value= %s\n", value);

		currPair = malloc(sizeof(currPair));
printf("namallod\n");
		if (currPair == NULL) printf("\nMalloc Error\n");
		currPair->key = type;
		currPair->value = value;
		currPair->next = NULL;


printf("CURR key= %i value= %s\n", type, value);
		//add cuurPair to the linked list
		if (firstPair == NULL) {
			firstPair = currPair;
			prevPair = currPair;
		}
		else {
			prevPair->next = currPair;
			prevPair = currPair;
		}
		if (sep != end)
			walker += strlen(SEP);
	}	
	return firstPair;
}

Originally Posted by django

I would still like to know if the thinking behind my malloc's (above) is seriously flawed.

Yep, sure is...

You live in a town where all car owners must have a garage ... (C requires you to reserve memory before use)
You do not have a garage. ( You haven't allocated any memory for storage)
You just went out and bought a car (You have data you need to store)

What do you have to do now?

That's right... you need to build a garage that will hold your car. (You need to allocate memory)

So, time passes and you decide you don't want a car anymore. (You're done with the data)
Now you can tear down that ugly garage. (You can free the memory you allocated)


Really... it's no more complex than that.
Just make sure you allocate enough memory for each object and free it up when you're done.