malloc 指针地址更改导致段错误？

Question

我尝试使用 C 编写一个简单的数据库。但是，我尝试调试我的分段错误并发现通过 malloc 获得的内存指针似乎在变化（名称和电子邮件指针似乎在 Database_load 程序执行之前和之后指向不同的内存位置)。我有两个问题：

1. 为什么执行 Database_load 前后内存指针（姓名和电子邮件）指向不同的位置？

2. 为什么程序会产生段错误？

这里是与问题相关的代码

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

int MAX_DATA;
int MAX_ROWS;

struct Address {
    int id;
    int set;
    //int MAX_DATA;
    char *name;
    char *email;
};

struct Database {
    //int MAX_ROWS;
    struct Address *rows;
};

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

void die(const char *message){
    if(errno){
        //perror(message);
        printf("ERROR: %s\n", 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 i;
    int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);
    if(rc != 1) die("Failed to load database.");

    for (i = 0; i < MAX_ROWS; i++) {
    printf("test Database_load loop read rows %p\n", &conn->db->rows[i]);   
    printf("test Database_load loop read rows name %p\n", &conn->db->rows[i].name); 
    printf("test Database_load loop read rows email %p\n", &conn->db->rows[i].email);   
    printf("test Database_load loop read rows name %s\n", conn->db->rows[i].name);  
    printf("test Database_load loop start %d\n", i);    
        rc = fread(&conn->db->rows[i], sizeof(struct Address), 1, conn->file);
    printf("test Database_load loop read rows %d\n", i);    
      rc = fread(&conn->db->rows[i].name, sizeof(MAX_DATA), 1, conn->file);
    printf("test Database_load loop read name %d\n", i);    
      rc = fread(&conn->db->rows[i].email, sizeof(MAX_DATA), 1, conn->file);
    printf("test Database_load loop read email %d\n", i);   
      if(rc != 1) die("Failed to load database.");
    printf("test Database_load loop\n");    
    }
}

struct Connection *Database_open(const char *filename, char mode){
    int i = 0;
    struct Connection *conn = malloc(sizeof(struct Connection));
    if(!conn) die("Memory error no connection");;

    conn->db = malloc(sizeof(struct Database));
    if(!conn->db) die("Memory error no database");

    conn->db->rows = malloc(sizeof(struct Address) * MAX_ROWS);
    if (conn->db->rows == NULL) die("No memory for rows");
    for(i = 0; i < MAX_ROWS; i++){
        // make a prototype to initialize it
        //struct Address addr = {.id = i, .set = 0};
        conn->db->rows[i].id = i;
        conn->db->rows[i].set = 0;
        conn->db->rows[i].name = malloc(sizeof(char) * MAX_DATA);
      if (conn->db->rows[i].name == NULL) die("No memory for name");
        conn->db->rows[i].email = malloc(sizeof(char) * MAX_DATA);
      if (conn->db->rows[i].email == NULL) die("No memory for email");
        // then just assign it
        if (i == 0) {
      printf("test set name = %p\n", &conn->db->rows[i].name);
      printf("test set email = %p\n", &conn->db->rows[i].email);
        }
    }

    if(mode == 'c'){
        conn->file = fopen(filename, "w");
    }
    else{
        conn->file = fopen(filename, "r+"); //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){
    int i = 0;
    rewind(conn->file);
    int rc = fwrite(conn->db, sizeof(struct Database), 1, conn->file);
    if(rc != 1) die("Failed to write database.");
    for (i = 0; i < MAX_ROWS; i++) {
        rc = fwrite(&conn->db->rows[i], sizeof(struct Address), 1, conn->file);

        if(rc != 1) die("Failed to write database.");
        rc = fwrite(&conn->db->rows[i].name, sizeof(MAX_DATA), 1, conn->file);
        if(rc != 1) die("Failed to write database.");
        rc = fwrite(&conn->db->rows[i].email, sizeof(MAX_DATA), 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 MAX_DATA, int MAX_ROWS){
    int i = 0;
    conn->db->rows = malloc(sizeof(struct Address) * MAX_ROWS);
    if (conn->db->rows == NULL) die("No memory for rows");
    for(i = 0; i < MAX_ROWS; i++){
        // make a prototype to initialize it
        struct Address addr = {.id = i, .set = 0};
        addr.name = malloc(sizeof(char) * MAX_DATA);
      if (addr.name == NULL) die("No memory for name");
        addr.email = malloc(sizeof(char) * MAX_DATA);
      if (addr.email == NULL) die("No memory for email");
        // 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];
    if(addr->set) die("Already set, delete it first");

    addr->set = 1;
    // warning: intentional bug, no relevant this question
    char *res = strncpy(addr->name, name, MAX_DATA);
    // 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> <MAX_ROWS> <MAX_DATA> [action params]");
    char *filename = argv[1];
    char action = argv[2][0];
    MAX_DATA = atoi(argv[3]);
    MAX_ROWS = atoi(argv[4]);

    int id = 0;
  if(argc > 5) id = atoi(argv[5]);
    struct Connection *conn = Database_open(filename, action);

    // legacy code, does not apply for create case
//  if(argc > 3) id = atoi(argv[3]);
//  if(id >= MAX_ROWS) die("There's not that many records.");

    switch(action){
        case 'c':
            if(argc != 5) die("Need MAX_DATA and MAX_ROWS");
            Database_create(conn, MAX_DATA, MAX_ROWS);
            Database_write(conn);
            break;

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

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

        case 'd':
            if(argc != 6) 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;
}

这是我执行程序后的 printf 输出

$./ex17_arbitrary db_arbitrary.dat c 512 100
$./ex17_arbitrary db_arbitrary.dat s 512 100 1 zed zed@zedshaw.com

test set name = 0x15ad058
test set email = 0x15ad060
test Database_load loop read rows (nil)
test Database_load loop read rows name 0x8
test Database_load loop read rows email 0x10

我确实注意到的一件事是，这两行在使用相同命令的多次执行中永远不会改变

test Database_load loop read rows name 0x8
test Database_load loop read rows email 0x10

更新： 我还有一些额外的设计问题。看起来当前数据结构的设计是有问题的。我将在这里详细说明设计要求：

除了我创建的功能之外，我不需要任何额外的功能。数据库的大小（MAX_DATA 和 MAX_ROWS 必须是可变的）。现在我每次调用程序时都在输入 MAX_DATA 和 MAX_ROWS。这可以改进吗？我在想当我需要使用 Database_create 方法时可能只给出 MAX_DATA 和 MAX_ROWS 。这个程序来自 (c.learncodethehardway.org/book/ex17.html) 中的一个有趣的练习，原始程序有一个固定大小的数据库。目标是使其变成可变大小。

Answer 1

在Database_load，你正在做：

int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);

但是，conn->db 是指向 struct Database 类型的指针，其唯一元素是：

struct Address *rows;

简而言之，您正在尝试从文件的fread 初始化指针 rows。这不是rows 指向的缓冲区/数组，而是rows 指针变量本身的内容（即rows 指向的内存地址）

由于您已经在 Database_open 中初始化了 rows，因此 fread 看起来很可疑，因为：

1. 你已经设置了rows

2. 做（例如）void *ptr = ...; read(fd,&ptr,sizeof(ptr)); 几乎永远不会正确。

3. 更正常的用法是：void *ptr = ...; read(fd,ptr,some_length);

通过执行上述 (2) 的等效操作，您正在覆盖 [垃圾] 在 Database_open 中初始化的 rows 值。就像你写的一样[糟糕]：

conn->db->rows = NULL;

或者：

conn->db->rows = (void *) 0x1234;

我不完全确定，因为没有数据我无法测试程序，但你也许可以简单地删除上面的fread。或者，如果在实际行数据之前的数据库中确实存在某种标题，则必须将其替换为其他内容。

但是，如果您将 fread 取出，rows 将保持不变，并且它指向的内容将像现在一样填充到 for 循环中。

---

更新：

我看到了问题。我认为这是一个更糟糕的设计。基本上，我将指针存储到数据库中并尝试将其读出并在不同的程序执行中访问相同的指针地址。

我在原始帖子中提到了这一点，但在我的编辑中将其删除，因为我认为您没有尝试这样做，而 fread 更像是一个“错字”。

但是，尝试将指针的持久值存储在文件中并在下次调用时恢复它们是一种糟糕的设计。

如果指针来自malloc，尤其如此。在第二次调用时，旧指针可能与 malloc 发生冲突。而且，您如何告诉malloc 旧指针现在以某种方式“保留”了？

对于指向全局/静态内存的指针，如果您重新构建程序并添加一个新变量[改变所有内容的地址和偏移]，会发生什么？

简而言之，您不能这样做 [这也是长答案 :-)]。

我能想到的解决方案是只存储struct Address，以及name字符串，address字符串。那会是更好的设计吗？

是的，如果您的意思如下：

struct Address {
    int id;
    int set;
    char name[MAX_DATA];
    char email[MAX_DATA];
};

现在，struct Address 可以读取/写入文件，因为它有 no 指针（即，这是关键）

再举一个例子，考虑一下如果struct Address 有一个嵌入的链表指针会发生什么：

struct Address {
    struct Address *link;
    int id;
    int set;
    char name[MAX_DATA];
    char email[MAX_DATA];
};

// simple traversal
for (addr = addrlist;  addr != NULL;  addr = addr->next) {
    // do stuff ...

    // broken, because struct Address has a pointer
    fwrite(addr,sizeof(struct Address),fout);
}

// fix for above
for (addr = addrlist;  addr != NULL;  addr = addr->next) {
    // do stuff ...

    fwrite(addr->id,sizeof(addr->id),fout);
    fwrite(addr->set,sizeof(addr->set),fout);
    fwrite(addr->name,sizeof(addr->name),fout);
    fwrite(addr->email,sizeof(addr->email),fout);
}

这是一种更独立的方式来完成列表：

struct Address {
    int id;
    int set;
    char name[MAX_DATA];
    char email[MAX_DATA];
};

// NOTE: either of these works
#if 1
struct AddrLink {
    struct AddrLink *link;
    struct Address *data;
};

#else
struct AddrLink {
    struct AddrLink *link;
    struct Address data;
};
#endif

// indirect list traversal
for (link = addrlist;  link != NULL;  link = link->next) {
    // do stuff ...

    // works because struct Address does _not_ have a pointer
    fwrite(link->data,sizeof(struct Address),fout);
}

一般情况下，您要做的类似于序列化。这是一个链接：C - serialization techniques 这是另一个：Serialize Data Structures in C

当我这样做时，我喜欢在数据前面加上一个标准的“节”标题。 .mp4、.avi 文件中使用了类似的技术：

struct section {
    int type;                           // section type
    int totlen;                         // total section length
    int size;                           // sizeof of section element
    int count;                          // number of array elements
};

#define TYPE_ADDRESS    1
#define TYPE_CITY       2
#define TYPE_STATE      3
#define TYPE_COUNTRY    4

这样，如果您的程序不理解新类型，因为它是旧版本，它仍然可以复制或跳过它不理解的数据而不会损害它。 （例如）这是处理 .mp4 文件时的必需行为。

---

更新 #2：

我已经发布了完整的代码。你能提出一个更好的设计方法吗？我对数据库文件的格式没有特定的约束

好的，下面的工作代码...

我用结构改变了一些东西[并重命名了它们]。值得注意的是，主结构[您称为Connection 现在称为database_t]。 Address 现在是 address_t。

你已经很接近了。你试图用你的 old struct Database 做什么，我用 dbheader_t 代替。也就是说，这些是我正在谈论的数据库头结构。你刚刚有指针。我在行数据开始之前将最大行数和最大数据记录为数据库文件的第一部分

我将分配代码移到了一个新函数Database_alloc [因为它现在必须在两个不同的地方调用]。

Database_open 必须稍微聪明一点。对于 c 操作，它会填充 DB 标头。对于所有其他操作，它必须打开数据库文件并读取磁盘头。

另外，使用新的结构组织，而不是到处使用 conn->db->rows，现在是 db->rows。

总的来说，你已经很接近了。

我还重新设计了main，使其更加用户友好（即您只需在c 命令中输入MAX_DATA/MAX_ROWS。

无论如何，这里是[请原谅无偿的风格清理]：

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

// database element
typedef struct _address {
    int id;                             // ID/slot number
    int set;                            // 1=active, 0=free/available
    char *name;                         // person's name
    char *email;                        // person's email address
} address_t;

// database on-disk header
typedef struct _header {
    int max_rows;                       // maximum number of rows
    int max_data;                       // maximum size of a field
} dbheader_t;
// NOTE: other stuff can be added (e.g. split max_data into max_name and
// max_email so that each field can have its own maximum length)

// database control
typedef struct _database {
    FILE *file;                         // database I/O stream
    dbheader_t header;                  // copy of on-disk header
    address_t *rows;                    // database data
} database_t;

void
die(const char *message)
{
    if (errno) {
        // perror(message);
        printf("ERROR: %s\n", message);
    }
    else {
        printf("ERROR: %s\n", message);
    }
    exit(1);
}

void
Address_print(address_t *addr)
{
    printf("%d %s %s\n",addr->id, addr->name, addr->email);
}

void
Database_load(database_t *db)
{
    int i;
    address_t *addr;
    int rc;

    // NOTE: database header has _already_ been read
#if 0
    rc = fread(db->file, sizeof(dbheader_t), 1, db->file);
    if (rc != 1)
        die("Failed to write database.");
#endif

    for (i = 0; i < db->header.max_rows; i++) {
        addr = &db->rows[i];

        rc = fread(&addr->id, sizeof(addr->id), 1, db->file);
        if (rc != 1)
            die("Failed to write database.");

        rc = fread(&addr->set, sizeof(addr->set), 1, db->file);
        if (rc != 1)
            die("Failed to write database.");

        rc = fread(addr->name, db->header.max_data, 1, db->file);
        if (rc != 1)
            die("Failed to write database.");

        rc = fread(addr->email, db->header.max_data, 1, db->file);
        if (rc != 1)
            die("Failed to write database.");
    }
}

void
Database_alloc(database_t *db)
{
    address_t *addr;

    db->rows = malloc(sizeof(address_t) * db->header.max_rows);
    if (db->rows == NULL)
        die("No memory for rows");

    for (int i = 0; i < db->header.max_rows; i++) {
        addr = &db->rows[i];

        // NOTE: no need to do it this way
        // make a prototype to initialize it
        // struct Address addr = {.id = i, .set = 0};

        addr->id = i;
        addr->set = 0;

        addr->name = calloc(db->header.max_data,sizeof(char));
        if (addr->name == NULL)
            die("No memory for name");

        addr->email = calloc(db->header.max_data,sizeof(char));
        if (addr->email == NULL)
            die("No memory for email");
    }
}

database_t *
Database_open(const char *filename, char mode, int max_rows, int max_data)
{
    int rc;

    database_t *db = calloc(1,sizeof(database_t));
    if (!db)
        die("Memory error no db pointer");

    switch (mode) {
    case 'c':
        db->file = fopen(filename, "w");
        if (!db->file)
            die("Failed to open the file");

        // set up a header [to write out]
        db->header.max_rows = max_rows;
        db->header.max_data = max_data;

        Database_alloc(db);
        break;

    default:
        db->file = fopen(filename, "r+");   // r+?
        if (!db->file)
            die("Failed to open the file");

        // read in header so we know the number of rows and the max data size
        rc = fread(&db->header,sizeof(dbheader_t),1,db->file);
        if (rc != 1)
            die("Failed to read header.");

        Database_alloc(db);
        Database_load(db);
    }

    return db;
}

void
Database_close(database_t *db)
{
    address_t *addr;

    if (db) {
        if (db->file)
            fclose(db->file);
        db->file = NULL;

        if (db->rows) {
            for (int rowidx = 0;  rowidx < db->header.max_rows;  ++rowidx) {
                addr = &db->rows[rowidx];
                free(addr->name);
                free(addr->email);
            }
            free(db->rows);
            db->rows = NULL;
        }

        free(db);
    }
}

void
Database_write(database_t *db)
{
    int i;
    int rc;
    address_t *addr;

    rewind(db->file);

    // write out the DB header
    rc = fwrite(&db->header, sizeof(dbheader_t), 1, db->file);
    if (rc != 1)
        die("Failed to write database.");

    for (i = 0; i < db->header.max_rows; i++) {
        addr = &db->rows[i];

        rc = fwrite(&addr->id, sizeof(addr->id), 1, db->file);
        if (rc != 1)
            die("Failed to write database.");

        rc = fwrite(&addr->set, sizeof(addr->set), 1, db->file);
        if (rc != 1)
            die("Failed to write database.");

        rc = fwrite(addr->name, db->header.max_data, 1, db->file);
        if (rc != 1)
            die("Failed to write database.");

        rc = fwrite(addr->email, db->header.max_data, 1, db->file);
        if (rc != 1)
            die("Failed to write database.");
    }

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

void
Database_set(database_t *db, int id, const char *name, const char *email)
{
    address_t *addr = &db->rows[id];

    if (addr->set)
        die("Already set, delete it first");
    addr->set = 1;

    // warning: intentional bug, no relevant this question
    // demonstrate the strncpy bug
    char *res = strncpy(addr->name, name, db->header.max_data);
    if (!res)
        die("Name copy failed");
    addr->name[db->header.max_data - 1] = 0;

    res = strncpy(addr->email, email, db->header.max_data);
    if (!res)
        die("Email copy failed");
    addr->email[db->header.max_data - 1] = 0;
}

void
Database_get(database_t *db, int id)
{
    address_t *addr = &db->rows[id];

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

void
Database_delete(database_t *db, int id)
{

    // NOTE/BUG: this causes a memory leak because it overwrites the name and
    // email fields without freeing them first
#if 0
    struct Address addr = {.id = id,.set = 0 };
    db->rows[id] = addr;
#else
    address_t *addr = &db->rows[id];
    addr->id = 0;
    addr->set = 0;
    memset(addr->name,0,db->header.max_data);
    memset(addr->email,0,db->header.max_data);
#endif
}

void
Database_list(database_t *db)
{
    int i;

    for (i = 0; i < db->header.max_rows; i++) {
        address_t *cur = &db->rows[i];
        if (cur->set) {
            Address_print(cur);
        }
    }
}

int
main(int argc, char *argv[])
{
    int max_data = 0;
    int max_rows = 0;
    int id = -1;

    if (argc < 3) {
        printf("USAGE: ex17 <dbfile> <action> [action params]");
        printf("  actions:\n");
        printf("    c <MAX_DATA> <MAX_ROWS> -- create database\n");
        printf("    g <id> -- get id and print\n");
        printf("    s <id> <name> <email> -- set id\n");
        printf("    d <id> -- delete id\n");
        printf("    l -- list database\n");
        die("aborting");
    }

    // skip over program name
    --argc;
    ++argv;

    --argc;
    char *filename = *argv++;

    --argc;
    char action = argv[0][0];
    ++argv;

    switch (action) {
    case 'c':
        if (argc != 2)
            die("Need MAX_DATA and MAX_ROWS");
        max_data = atoi(argv[0]);
        max_rows = atoi(argv[1]);
        break;
    }

    database_t *db = Database_open(filename, action, max_rows, max_data);

    // legacy code, does not apply for create case
//  if(argc > 3) id = atoi(argv[3]);
//  if(id >= db->header.max_rows) die("There's not that many records.");

    switch (action) {
    case 'c':
        Database_write(db);
        break;

    case 'g':
        if (argc != 1)
            die("Need an id to get");
        id = atoi(argv[0]);
        Database_get(db, id);
        break;

    case 's':
        if (argc != 3)
            die("Need id, name, email to set");
        id = atoi(argv[0]);
        Database_set(db, id, argv[1], argv[2]);
        Database_write(db);
        break;

    case 'd':
        if (argc != 1)
            die("Need id to delete");
        id = atoi(argv[0]);
        Database_delete(db, id);
        Database_write(db);
        break;

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

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

    Database_close(db);

    return 0;
}