From 9fc454c84d539cd90aed3a74a350bdc792455407 Mon Sep 17 00:00:00 2001 From: Dmitri Pal Date: Tue, 14 Apr 2009 14:55:42 -0400 Subject: Fixing memory issues in ini and collection The read_line() function used an internal buffer allocated on stack as temporary storage for a line read from file, then returned it. read_line() now gets a buffer from the caller. Fixed memory leaks in INI and Collection found by valgrind. --- common/collection/collection_ut.c | 15 +++++++++------ common/ini/ini_config.c | 36 +++++++++++++++++++++++++++++------- common/ini/ini_config_ut.c | 4 ++-- 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/common/collection/collection_ut.c b/common/collection/collection_ut.c index 6d27db66d..eabf52227 100644 --- a/common/collection/collection_ut.c +++ b/common/collection/collection_ut.c @@ -480,6 +480,7 @@ int mixed_collection_test() /* Traverse collection again - peer should still be there */ error = print_collection(event); if(error) { + destroy_collection(event); printf("Error printing collection %d\n",error); return error; } @@ -488,18 +489,17 @@ int mixed_collection_test() error = debug_collection(event,COL_TRAVERSE_DEFAULT); if(error) { + destroy_collection(event); printf("Error printing collection %d\n",error); return error; } printf("Attempt to add property to a referenced collection.\n"); - /* Some negative tests */ - /* Can't add attributes to the referenced collection */ error = add_int_property(event,"host","session",500); - if(error != 0) printf("Error was NOT able to add property to a referenced collection.\n"); - else { - printf("Unexpected success which is an implementation error.\n"); + if(error) { + destroy_collection(event); + printf("Error was NOT able to add property to a referenced collection %d.\n", error); return error; } @@ -508,6 +508,7 @@ int mixed_collection_test() /* Can't delete non exitent property */ error = delete_property(event,"host.host",COL_TYPE_ANY, COL_TRAVERSE_DEFAULT); if(error == 0) { + destroy_collection(event); printf("Error was able to delete property that does not exist.\n"); return -1; } @@ -516,12 +517,14 @@ int mixed_collection_test() /* Set collection class */ error = set_collection_class(event,2); if(error != 0) { + destroy_collection(event); printf("Error was NOT able to set class.\n"); return error; } error = get_collection_class(event,&class); if(error != 0) { + destroy_collection(event); printf("Error was NOT able to get class.\n"); return error; } @@ -529,6 +532,7 @@ int mixed_collection_test() if(is_of_class(event,2)) printf("Class mathced!\n"); else { + destroy_collection(event); printf("Error - bad class.\n"); return -1; } @@ -706,4 +710,3 @@ int main() /* Add other tests here ... */ return error; } - diff --git a/common/ini/ini_config.c b/common/ini/ini_config.c index 4112049f3..fd1efb026 100644 --- a/common/ini/ini_config.c +++ b/common/ini/ini_config.c @@ -83,8 +83,14 @@ inline const char *parsing_error_str(int parsing_error) return str_error[parsing_error-1]; } - -int read_line(FILE *file,char **key,char **value, int *length, int *ext_error); +/* Internal function to read line from INI file */ +int read_line(FILE *file, + char *buf, + int read_size, + char **key, + char **value, + int *length, + int *ext_error); /* Add to collection or update - CONSIDER moving to the collection.c */ static int add_or_update(struct collection_item *current_section, @@ -137,6 +143,8 @@ static int ini_to_collection(const char *filename, struct parse_error pe; int line = 0; int created = 0; + char buf[BUFFER_SIZE+1]; + TRACE_FLOW_STRING("ini_to_collection", "Entry"); @@ -162,7 +170,8 @@ static int ini_to_collection(const char *filename, /* Read file lines */ while (1) { - status = read_line(file, &key, &value, &length, &ext_err); + /* Always read one less than the buffer */ + status = read_line(file, buf, BUFFER_SIZE+1, &key, &value, &length, &ext_err); if (status == RET_EOF) break; line++; @@ -505,11 +514,15 @@ int config_for_app(const char *application, } /* Reads a line from the file */ -int read_line(FILE *file, char **key,char **value, int *length, int *ext_error) +int read_line(FILE *file, + char *buf, + int read_size, + char **key, char **value, + int *length, + int *ext_error) { char *res; - char buf[BUFFER_SIZE+1]; int len; char *buffer; int i; @@ -522,12 +535,15 @@ int read_line(FILE *file, char **key,char **value, int *length, int *ext_error) buffer = buf; /* Get data from file */ - res = fgets(buffer, BUFFER_SIZE, file); + res = fgets(buffer, read_size - 1, file); if (res == NULL) { TRACE_ERROR_STRING("Read nothing", ""); return RET_EOF; } + /* Make sure the buffer is NULL terminated */ + buffer[read_size - 1] = '\0'; + len = strlen(buffer); if (len == 0) { TRACE_ERROR_STRING("Nothing was read.", ""); @@ -550,7 +566,8 @@ int read_line(FILE *file, char **key,char **value, int *length, int *ext_error) TRACE_INFO_STRING("BUFFER before trimming:", buffer); /* Trucate trailing spaces and CRs */ - while (isspace(buffer[len - 1])) { + /* Make sure not to step before the beginning */ + while (len && isspace(buffer[len - 1])) { buffer[len - 1] = '\0'; len--; } @@ -847,6 +864,9 @@ int get_config_item(const char *section, error = get_item(section_handle, name, COL_TYPE_STRING, COL_TRAVERSE_ONELEVEL, item); + /* Make sure we free the section we found */ + destroy_collection(section_handle); + TRACE_FLOW_NUMBER("get_config_item returning", error); return error; } @@ -1521,6 +1541,8 @@ char **get_attribute_list(struct collection_item *ini_config, const char *sectio /* Pass it to the function from collection API */ list = collection_to_list(subcollection, size, error); + destroy_collection(subcollection); + TRACE_FLOW_STRING("get_attribute_list returning", list == NULL ? "NULL" : list[0]); return list; } diff --git a/common/ini/ini_config_ut.c b/common/ini/ini_config_ut.c index 6787c36c2..5441e02c0 100644 --- a/common/ini/ini_config_ut.c +++ b/common/ini/ini_config_ut.c @@ -303,7 +303,6 @@ int get_test() debug_item(item); - printf("Get item as string without duplication from NULL item.\n"); /* Get a string without duplicication */ @@ -759,9 +758,10 @@ int get_test() return -1; } - for (i=0;i