Code:
alex@alex-VirtualBox:~/Documents/Learning to C$ valgrind --leak-check=full ./ex17 alloctest4 c
==4726== Memcheck, a memory error detector
==4726== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==4726== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==4726== Command: ./ex17 alloctest4 c
==4726==
The pointer to name is 0x41f0120
The pointer to email is 0x41f0158
The pointer to name is 0x41f0190
The pointer to email is 0x41f01c8
The pointer to name is 0x41f0200
The pointer to email is 0x41f0238
The pointer to name is 0x41f0270
The pointer to email is 0x41f02a8
The pointer to name is 0x41f02e0
The pointer to email is 0x41f0318
Name addr: 0x41f0120
Email addr: 0x41f0158
String size: 5
==4726== Invalid read of size 1
==4726== at 0x40B56C8: _IO_file_xsputn@@GLIBC_2.1 (fileops.c:1362)
==4726== by 0x40AA7F7: fwrite (iofwrite.c:45)
==4726== by 0x8048C17: Database_write (ex17.c:140)
==4726== by 0x8048FA8: main (ex17.c:229)
==4726== Address 0x41f0125 is 0 bytes after a block of size 5 alloc'd
==4726== at 0x402BD74: malloc (vg_replace_malloc.c:270)
==4726== by 0x8048872: Database_open (ex17.c:75)
==4726== by 0x8048F46: main (ex17.c:220)
==4726==
==4726== Invalid read of size 1
==4726== at 0x40B56D4: _IO_file_xsputn@@GLIBC_2.1 (fileops.c:1361)
==4726== by 0x40AA7F7: fwrite (iofwrite.c:45)
==4726== by 0x8048C17: Database_write (ex17.c:140)
==4726== by 0x8048FA8: main (ex17.c:229)
==4726== Address 0x41f0126 is 1 bytes after a block of size 5 alloc'd
==4726== at 0x402BD74: malloc (vg_replace_malloc.c:270)
==4726== by 0x8048872: Database_open (ex17.c:75)
==4726== by 0x8048F46: main (ex17.c:220)
==4726==
==4726== Invalid read of size 1
==4726== at 0x40B56C8: _IO_file_xsputn@@GLIBC_2.1 (fileops.c:1362)
==4726== by 0x40AA7F7: fwrite (iofwrite.c:45)
==4726== by 0x8048C6D: Database_write (ex17.c:142)
==4726== by 0x8048FA8: main (ex17.c:229)
==4726== Address 0x41f015d is 0 bytes after a block of size 5 alloc'd
==4726== at 0x402BD74: malloc (vg_replace_malloc.c:270)
==4726== by 0x80488DB: Database_open (ex17.c:79)
==4726== by 0x8048F46: main (ex17.c:220)
==4726==
==4726== Invalid read of size 1
==4726== at 0x40B56D4: _IO_file_xsputn@@GLIBC_2.1 (fileops.c:1361)
==4726== by 0x40AA7F7: fwrite (iofwrite.c:45)
==4726== by 0x8048C6D: Database_write (ex17.c:142)
==4726== by 0x8048FA8: main (ex17.c:229)
==4726== Address 0x41f015e is 1 bytes after a block of size 5 alloc'd
==4726== at 0x402BD74: malloc (vg_replace_malloc.c:270)
==4726== by 0x80488DB: Database_open (ex17.c:79)
==4726== by 0x8048F46: main (ex17.c:220)
==4726==
Name addr: 0x41f0190
Email addr: 0x41f01c8
String size: 5
Name addr: 0x41f0200
Email addr: 0x41f0238
String size: 5
Name addr: 0x41f0270
Email addr: 0x41f02a8
String size: 5
Name addr: 0x41f02e0
Email addr: 0x41f0318
String size: 5
==4726== Syscall param write(buf) points to uninitialised byte(s)
==4726== at 0x4122443: __write_nocancel (syscall-template.S:82)
==4726== by 0x40B4BA4: _IO_file_write@@GLIBC_2.1 (fileops.c:1289)
==4726== by 0x40B4A83: new_do_write (fileops.c:543)
==4726== by 0x8048FA8: main (ex17.c:229)
==4726== Address 0x403505c is not stack'd, malloc'd or (recently) free'd
==4726==
==4726==
==4726== HEAP SUMMARY:
==4726== in use at exit: 0 bytes in 0 blocks
==4726== total heap usage: 14 allocs, 14 frees, 502 bytes allocated
==4726==
==4726== All heap blocks were freed -- no leaks are possible
==4726==
==4726== For counts of detected and suppressed errors, rerun with: -v
==4726== Use --track-origins=yes to see where uninitialised values come from
==4726== ERROR SUMMARY: 151 errors from 5 contexts (suppressed: 0 from 0)
As you can see, fwrite seems to be trying to read from memory at the end of the allocated space in the first iteration. This causes a segfault when run without valgrind. In subsequent iterations, it appears to run without problems, but I think this is an artifact of valgrind, as starting the Database_write loop from i=1 results in the same behaviour (first iteration tries to use the wrong memory).
Code:
#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;
char *email;
};
struct Database {
int numrows;
int stringsize;
struct Address *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);
}
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.");
int i = 0;
for(i = 0; conn->db->numrows; i++){
int rowc = fread(&conn->db->rows[i], sizeof(struct Address)+2*conn->db->stringsize, 1, conn->file);
if(rowc != 1) die("Failed to write row");
}
}
struct Connection *Database_open(const char *filename, char mode, int numrows, int stringsize)
{
struct Connection *conn = malloc(sizeof(struct Connection));
if(!conn) die("Memory error");
conn->db = malloc(sizeof(struct Database));
conn->db->numrows = numrows;
conn->db->stringsize = stringsize;
if(!conn->db) die("Memory error");
//Allocate memory for a set of Address pointers
conn->db->rows = malloc(sizeof(struct Address)*numrows);
if(!conn->db->rows) die("Memory error in row creation");
//Allocate the memory for each string field of the each Address struct and
//assign.
int i = 0;
for(i = 0; i < numrows; i++){
conn->db->rows[i].name = malloc(sizeof(char)*stringsize);
printf("The pointer to name is %p\n",conn->db->rows[i].name);
if(!conn->db->rows[i].name) die("Memory error in name field creation");
conn->db->rows[i].email = malloc(sizeof(char)*stringsize);
printf("The pointer to email is %p\n",conn->db->rows[i].email);
if(!conn->db->rows[i].email) die("Memory error in email field creation");
conn->db->rows[i].id = i;
conn->db->rows[i].set = 0;
}
if(mode == 'c') {
conn->file = fopen(filename, "w");
} 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)
{
int i = 0;
if(conn) {
if(conn->file) fclose(conn->file);
if(conn->db->rows){
for(i=0;i<conn->db->numrows;i++){
if(conn->db->rows[i].name) free(conn->db->rows[i].name);
if(conn->db->rows[i].email) free(conn->db->rows[i].email);
}
free(conn->db->rows);
}
if(conn->db) free(conn->db);
free(conn);
}
}
void Database_write(struct Connection *conn)
{
rewind(conn->file);
/* File structure
Database and size information (extra pointer value written)
Row information (2 x extra pointer values written)
*/
int rc = fwrite(conn->db, sizeof(struct Database), 1, conn->file);
if(rc != 1) die("Failed to write db to database.\n");
int sc = fwrite(conn->db->rows, sizeof(struct Address)*conn->db->numrows,1,conn->file);
if(sc !=1) die("Failed to write rows to database.\n");
int i =0;
for(i = 0; i<conn->db->numrows; i++){
printf("Name addr:\t%p\n",conn->db->rows[i].name);
printf("Email addr:\t%p\n",conn->db->rows[i].email);
printf("String size:\t%d\n",conn->db->stringsize);
int rowname = fwrite(conn->db->rows[i].name, (sizeof(char*)*(conn->db->stringsize)), 1, conn->file);
if(rowname != 1) die("Failed to write name row");
int rowemail = fwrite(conn->db->rows[i].email, (sizeof(char*)*(conn->db->stringsize)), 1, conn->file);
if(rowemail != 1) die("Failed to write email row");
}
rc = fflush(conn->file);
if(rc == -1) die("Cannot flush database.");
}
void Database_create(struct Connection *conn)
{
int i = 0;
for(i = 0; i < conn->db->numrows; i++) {
// make a prototype to initialize it
struct Address addr = {.id = i, .set = 0};
// then just assign it
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];
printf("The mem addr to set is %p\n",addr);
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, conn->db->stringsize);
// demonstrate the strncpy bug
if(!res) die("Name copy failed");
res = strncpy(addr->email, email, conn->db->stringsize);
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 < conn->db->numrows; 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]");
//Test constants
int numrows = 5;
int stringsize = 5;
char *filename = argv[1];
char action = argv[2][0];
struct Connection *conn = Database_open(filename, action, numrows, stringsize);
int id = 0;
if(argc > 3) id = atoi(argv[3]);
if(id >= numrows) 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, only: c=create, g=get, s=set, d=del, l=list");
}
Database_close(conn);
return 0;
}
Lastly, and as always, if anyone has any stylistic objections, or ways I could better do things (aside from the current problem I'm having), then I'd love to hear them.