Thread: Another question about dynamic allocation

Originally Posted by Subsonics

Well that's subtle, especially since the value of NULL is required to be 0.

No it is not.

The standard requires that the effect of assigning a pointer to zero is the same as assigning it to the NULL pointer.

There is no requirement that the NULL pointer has a zero value, nor that it is represented with "all bits zero", and there never has been.

Originally Posted by Subsonics

Yes it is. When I mentioned NULL (all caps) here, I'm referring to the null pointer constant.

I knew what you were referring to. You are still mistaken.

Originally Posted by Subsonics

Lets first establish what it is you are disagreeing with. "No it is not" subtle or "no it is not" required to be 0. Because there is nothing in post # 31 that I disagree with.

I disagree with your claim that a null pointer constant must have a value of zero.

Originally Posted by grumpy

I disagree with your claim that a null pointer constant must have a value of zero.

Hmm... this looks like an area where the standard could be interpreted either way, i.e., it could be defining what a null pointer constant is, or it could be merely mentioning that the given expressions are null pointer constants. The word "called" makes me side with the former though, i.e., a null pointer constant must have the value 0, otherwise the implementation is non-conforming. In any case, this is uselessly pedantic since where it matters, a null pointer constant would behave as if it has the value 0.

Originally Posted by Subsonics

But do you really think that is the intended meaning?

Yes, having worked with an embedded system, and development environment for that embedded system, where that meaning was exactly the case.

The standard specifically allows freedoms for implementations (i.e. compilers and host environments). In fact, the standard goes so far as to say that anything it does not specifically define is undefined (or words to that effect). If something is undefined, any interpretation that may occur in an implementation is correct.

The fact it is possible to detect - as you pointed out - that the wording does leave open the possibility, means it is permitted.

Originally Posted by phantomotap

So have I, and if the compiler for such a system didn't also allow the literal value of zero to be used as an alias for the system's null pointer value that system doesn't conform to the standard in this regard.

The particular one I used did allow zero to be used. There was also a particular non-zero value (which I'd have to look up, as I can't recall offhand) which would ALSO compare equal to null.

The implementation in question was standalone, not hosted.

Ok guys, I've been scratching my head over this in the evenings when I get home after work, and I can't figure out what's going on.

In the code brackets below, I show the valgrind output for my code when run.

Please note that the code is the same as the last linked version in the thread, with one exception. Database_create has been commented out (it was a superfluous initialisation step). I've attached the current working code at the bottom, hopefully minimised by default so it doesn't blow the post out to crazy huge proportions.

Here's the output.

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).

Does anyone have any idea why this would be happening?

Here's the code as it stands.

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.