Ex17 EXC_BAD_MEMORY_ACCESS

At the risk of being the noobest guy in the room, I need some help with ex17. After reading through the program numerous times and working through compile errors, I keep running into a segfault when running the program. Using lldb I see that during Database_create() initialization loop the program can’t find the passed structure when trying to assign address values:

void Database_create(struct Connection *conn){

	int i = 0;

	for(i = 0; i < MAX_ROWS; i++){
		//make a prototype to initialize it
		struct Address addr = {.id = i, .set = 0};
		//then just assign it
		conn->db->rows[i] = addr;  /*<-- throws an exception "EXC_BAD_MEMORY_ACCESS"*/
        printf("Location of conn->db->rows = %p\n", conn->db->rows); /*<-- outputs "Location of conn->db->rows = 0x0" */
		
	}
}

I also seem to get strange values during struct Connection *Database_open(), shown below:

struct Connection *Database_open(const char *filename, char mode){

	struct Connection *conn = malloc(sizeof(struct Connection));
 /*should be the size of everything inside *conn->db->addr -- (rows[MAX_DATA] + name[MAX_DATA] + int id + int set), right?  (103200 + 103200 + 4 + 4 = 103208)*/

	printf("sizeof malloc = %lu\n", sizeof(struct Connection)); 
//outputs "sizeof malloc = 16"

	printf("sizeof *conn = %lu\n", sizeof(conn));
//outputs "sizeof *conn = 8"

I’m new to C but have been able to at least verify the program creates rows[MAX_DATA], creates “struct Address addr” (and I can manipulate the values), but I cannot assign addr to a cell in rows[] to save my life. Anybody have a hint for me? I’ve even tried cursing at the screen (a lot), but it didn’t seem to fix anything.

Remember, helping a stranger is a great way to feel good about yourself! Thanks in advance.

It looks like you are accessing stuff outside of the conn->db->rows definition. Please post the complete source so I can have a look at whats wrong. I did this exercise a long time ago and had no issues with it. I also checked git diff for the changes I made to ex17.c and found nothing except for a replace of connect (so it made more sense to me) and some reformatting due to my vi configuration.

Post the complete source so I can have a look and see if I have the same problems.

Guus.

So, first up when you post code do this:

[code]
// my C code here
[/code]

I did that for you so we can read it. Now, next thing is, you’re running into one of the things I talk about in the course where what you get from the computer is never what you think it is. You think when you use malloc you get fresh, clean, ready to go memory? No way, this is C man. It gives you the bare minimum chunk of memory to use and it will be full of garbage from previous uses.

If you want clean memory you either have to clean it yourself, or use the calloc function instead.

Next, you have to find out which of your pointers in the conn->db->rows chain isn’t allocated or initialized correctly. Fire up gdb and set a breakpoint right before this error, or just run it inside gdb and then it’ll crash at that point but you can inspect it. I have a video up where I show how to do this. Once you have that, you’ll be able to inspect that pointer chain and figure out where it’s wrong. Most likely it’s because you’re using malloc instead of calloc and that is setting fake values to these pointers so you don’t really set them.

Thanks for the help, Zed and Guus. As requested, I’ve posted my complete source code below. After reading Zed’s response I went back into ex17.c and replaced the malloc() call with calloc(1, sizeof(struct Connection)) but still get the same error message.

#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>

#define MAX_DATA 512
#define MAX_ROWS 100

struct Address {

	int id;
	int set;
	char name[MAX_DATA];
	char email[MAX_DATA];

};

struct Database {

	struct Address rows[MAX_ROWS];

};

struct Connection {
	
	FILE *file;
	struct Database *db;

};

void die(const char *message){

	if(errno){
		perror(message);
	}
	else{
		printf("ERROR: %s\n", message);
	}

	exit(1); //terminate program
}

void Address_print(struct Address *addr){

	printf("%d %s %s\n", addr->id, addr->name, addr->email);

}

void Database_load(struct Connection *conn){

	int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);
	if(rc != 1){
		die("Failed to load database.");
	}

}

struct Connection *Database_open(const char *filename, char mode){

	struct Connection *conn = calloc(1, sizeof(struct Connection));

	printf("sizeof calloc = %lu\n", sizeof(struct Connection));
	printf("sizeof *conn = %lu\n", sizeof(conn));

	if(!conn){
		die("Memory error");
	}

	if(mode == 'c'){
		conn->file = fopen(filename, "w");
		printf("I wrote a file!\n");
	}
	else{
		conn->file = fopen(filename, "r+");

		if(conn->file){
			Database_load(conn);
		}
	}

	if(!conn->file){
		die("Failed to open the file.");
	}

	return conn;
}

void Database_close(struct Connection *conn){

	if(conn){
		if(conn->file){
			fclose(conn->file);
		}
		if(conn->db){
			free(conn->db);
		}
		free(conn);
	}
}

void Database_write(struct Connection *conn){

	printf("TOP OF Database_write!!\n");

	rewind(conn->file);

	int rc = fwrite(conn->db, sizeof(struct Database), 1, conn->file);
	if(rc != 1){
		die("Failed to write database.");
	}

	rc = fflush(conn->file);
	if(rc == -1){
		die("Cannot flush database.");
	}
}

void Database_create(struct Connection *conn){

	int i = 0;

	printf("Does conn exist in Database_create()?  sizeof Connection = %lu, sizeof conn->db->rows = %lu\n", sizeof(struct Connection), sizeof(conn->db->rows));
	printf("Location of conn->db->rows = %p\n", conn->db->rows);

	for(i = 0; i < MAX_ROWS; i++){
		//make a prototype to initialize it
		struct Address addr = {.id = i, .set = 0};
		//then just assign it
		/*printf("addr[%d] ; id=%d, set=%d i, addr.id, addr.set);*/

		/*printf("conn->db->rows[i].id = %d\n\n", conn->db->rows[i].id);*/
		conn->db->rows[i] = addr;
		
	}
}

void Database_set(struct Connection *conn, int id, const char *name, const char *email){
		
	struct Address *addr = &conn->db->rows[id];
	if(addr->set){
		die("Already set, delete it first.");
	}
	addr->set = 1;
	//WARNING: bug, read the "How To Break It" and fix this
	char *res = strncpy(addr->name, name, MAX_DATA);
	//attempt to fix strncpy() bug
	res[(MAX_DATA - 1)] = '\0';//always set the end of res[] to a null bit with \0

	//demonstrate the strncpy bug
	if(!res){
		die("Name copy failed.");
	}

	res = strncpy(addr->email, email, MAX_DATA);
	if(!res){
		die("Email copy failed");
	}

}

void Database_get(struct Connection *conn, int id){

	struct Address *addr = &conn->db->rows[id];

	if(addr->set){
		Address_print(addr);
	}
	else{
		die("ID is not set.");
	}

}

void Database_delete(struct Connection *conn, int id){

	struct Address addr = {.id = id,.set = 0};
	conn->db->rows[id] = addr;

}

void Database_list(struct Connection *conn){

	int i = 0;
	struct Database *db = conn->db;

	for(i = 0; i < MAX_ROWS; i++){

		struct Address *cur = &db->rows[i];

		if(cur->set){
			Address_print(cur);
		}
	}

}

int main(int argc, char *argv[]){

	if(argc < 3){
		die("USAGE: ex17 <dbfile> <action> [action params]");
	}

	char *filename = argv[1];
	char action = argv[2][0];
	struct Connection *conn = Database_open(filename, action);
	printf("Does conn exist in main()?  sizeof Connection = %lu, sizeof conn = %lu\n", sizeof(struct Connection), sizeof(conn));
	printf("conn->db = %p\n", conn->db);
	printf("conn->db (sizeof) = %lu\n", sizeof(conn->db));
	printf("conn->db->rows (sizeof)= %lu\n", sizeof(conn->db->rows));
	int id = 0;

	if(argc > 3){
		id = atoi(argv[3]);
	}
	if(id >= MAX_ROWS){
		die("There's not that many records.");
	}

	switch (action) {

		case 'c':
			Database_create(conn);
			Database_write(conn);
			break;

		case 'g':
			if(argc != 4){
				die("Need an id to get");
			}
			Database_get(conn, id);
			break;

		case 's':
			if(argc != 6){
				die("Need id, name, email to set.");
			}
			Database_set(conn, id, argv[4], argv[5]);
			Database_write(conn);
			break;

		case 'd':
			if(argc != 4){
				die("Need id to delete.");
			}
			Database_delete(conn, id);
			Database_write(conn);
			break;

		case 'l':
			Database_list(conn);
			break;

		default:
			die("Invalid action: c=create, g=get, s=set, d=delete, l=list");
	}

	Database_close(conn);
	
	return 0;
}

When I use lldb to debug I see the following info (breakpoint set at Database_create(), stepping into problematic line 133):

  • thread #1: tid = 0x124c9, 0x000000010000164a ex17Database_create(conn=0x00000001001005b0) + 186 at ex17.c:133, queue = 'com.apple.main-thread', stop reason = step in frame #0: 0x000000010000164a ex17Database_create(conn=0x00000001001005b0) + 186 at ex17.c:133
    130 /printf("addr[%d] ; id=%d, set=%d i, addr.id, addr.set);/
    131
    132 /printf(“conn->db->rows[i].id = %d\n\n”, conn->db->rows[i].id);/
    -> 133 conn->db->rows[i] = addr;
    134
    135 }
    136 }
    (lldb) step
    Process 1489 stopped
  • thread #1: tid = 0x124c9, 0x00007fff8d62d5ea libsystem_platform.dylib_platform_memmove$VARIANT$Nehalem + 138, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) frame #0: 0x00007fff8d62d5ea libsystem_platform.dylib_platform_memmove$VARIANT$Nehalem + 138
    libsystem_platform.dylib`_platform_memmove$VARIANT$Nehalem:
    -> 0x7fff8d62d5ea <+138>: movups %xmm0, (%rax)
    0x7fff8d62d5ed <+141>: movups 0x10(%rsi), %xmm2
    0x7fff8d62d5f1 <+145>: movups 0x20(%rsi), %xmm3
    0x7fff8d62d5f5 <+149>: movups 0x30(%rsi), %xmm4

I hope this helps, thank you!

Ok, so now you just have to use the lldb commands to see why your conn variable is not what you think it is. And you have to look in other places not just where the error is. For example, I’m looking at your Database_open and I don’t see where you create the conn->db? From just my quick look you don’t create any memory for it so then when you print conn->db->rows it dies. You should also use lldb to print each of these: conn , conn->db, con->db->rows to see which one isn’t getting created.

Thanks for the help! I went back and checked each line of Database_open and you were right, I omitted the lined creating conn->db. Sometimes I just need a second set of eyes to show me the obvious mistake I overlooked (I swear I checked through the code at least twice before posting! Grr…).

The program is working as expected now. I’ll spend more time with lldb so I can make more sense of the debug info.

Thank you again for taking the time to help me out.

Nice, glad you figured it out.