C——在释放内存时将字符串数据插入链表会使程序崩溃

Question

StackOverflow 上的第一篇文章(贸易测试员，业余时间非常糟糕的程序员)。出于披露目的，这是我正在努力完成的一项大学作业(据我所知，在这里提问并不被禁止。

无论如何，我有一个程序可以从文本文件中读取一行，标记行数据，创建一个链接列表，然后将每个标记(2 个字符串、1 个 float 、1 个无符号)插入到节点中。在节点使用的内存被释放之前一切都很好——整个程序崩溃了。调试后，我似乎将问题隔离到两个字符串复制操作。它们看起来都非常有效，但 free() 根本不喜欢它们。试过 strncpy() - 没有区别。尝试逐个复制字符串 char - 没有区别。现在我不知所措......

下面的代码，如果有人想看一看(哦，更多的披露 - 几乎是一个完整的 C n00b，所以是的，如果你看到下面的不良做法，那就是我......)

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include "ts.h"
int main(void)
{
    /* Variables */
    FILE *stream;
    char buf[BUFSIZ];
    /* Open stock file */         //TODO - make this dynamic, don't hard-code file name
    stream = fopen("stock.csv", "r");
    assert(stream);
    /* Using addStockNode */
    while (fgets(buf, BUFSIZ, stream))
    {
        addStockNode(buf);
    }
}
void addStockNode(char* stockLine)
{
    //Create linked list
    StockNodePtr head, new, current, previous, nextStock;
    unsigned listSize;

    char *stkTok1, *stkTok2, *stkTok3, *stkTok4;
    unsigned stkLevel;
    int i;
    float stkPrice;

    listSize = 0;
    head = NULL;

        /* Create new stock node */
        if ((new = malloc(sizeof(StockNodePtr))) == NULL)
        {
            fprintf(stderr,"\nMemory allocation for node insertion failed\n");
            fprintf(stderr,"Aborting\n");
            exit(EXIT_FAILURE);
        }

        /* Tokenise data */
        stkTok1 = strtok(stockLine, ",");
        stkTok2 = strtok(NULL, ",");
        stkTok3 = strtok(NULL, ",");
        stkTok4 = strtok(NULL, ",");

        /* Search to find where in insert new list node */      //TODO - needs to be adapted to sort by stock DESCRIPTION
        current = head;
        previous = NULL;

        /* stockID */
     // strcpy(new->stockID, stkTok1);                 //falls over at free()
     // strncpy(new->stockID, stkTok1, STOCKID_LEN);   //falls over at free()

        for(i = 0; i < strlen(stkTok1); i++)           //still falls over at free()
        {
            new->stockID[i] = stkTok1[i];
        }

        /* description */
     // strcpy(new->description, stkTok2);              //falls over at free()

        /* unitPrice */
        stkPrice = strtof(stkTok3, NULL);
        new->unitPrice = stkPrice;

        /* StockLevel */
        stkLevel = strtol(stkTok4, NULL, 10);
        new->stockLevel = stkLevel;

        /*nextStock */
        new->nextStock = current;

        /* Increment listSize */
        listSize++;

        //TAKE OUT LATER - loadData can iterate through each line of the file */
        if(previous == NULL)            
        {
            head = new;
        }
        else
        {
            previous->nextStock = new;
        }

    /* Print node details */
    current = head;
    printf("%s,%s,%f,%i\n", current->stockID, current->description, current->unitPrice, current->stockLevel);       

    /* Deallocate memory used by node */
    current = head;
    while(current != NULL)
    {
        nextStock = current->nextStock;
        free(current);    //EXECUTE THIS, IT FALLS OVER (with strcpy lines uncommented)
        current = nextStock;
    }

    return EXIT_SUCCESS;
}*

为了完整起见，这里是库存节点结构...

typedef struct stockNode
{
   char stockID[STOCKID_LEN + 1];
   char description[DESCRIPTION_MAX + 1];
   float unitPrice;
   unsigned stockLevel;
   StockNodePtr nextStock;
} StockNodeType;

如果有人能指出我哪里出错了，我将不胜感激!

编辑 - 这是库存节点常量...

#define STOCKID_LEN 5
#define DESCRIPTION_MAX 40
#define PRICE_COLWIDTH 7
#define STOCKLEVEL_COLWIDTH 3
#define STOCKLEVEL_MAX 100

哦，还有正在添加的股票数据(并不是说它有什么问题)...

S0001,Slazenger Classic Racquet,150.00,5
S0002,Slazenger Lite Racquet,98.00,3
S0003,Wilson Tournament Gold Balls,14.95,20
S0004,Dunlop Grand Prix Balls,10.95,25
S0005,Luft Nemesis Racquet,125.00,1
S0006,Wilson Tournament Balls,12.95,12

Answer 1

主要缺陷

您的根本问题是分配不足，具体而言:

if ((new = malloc(sizeof(StockNodePtr))) == NULL)

这意味着您正在分配足够大的空间来容纳指针；不是存量节点。

试试这个:

if ((new = malloc(sizeof(*new))) == NULL)

注意:我没有彻底阅读其余部分，但这本身就相当庞大，因为您永远不会为每个节点分配超过指针大小的内存。如果还有其他问题，我会在看到它们时发表评论。

---

总体回顾

代码主体内的以下一般性意见和建议不一定与主要缺陷相关。然而，他们中的大多数指出了可能导致 future 缺陷的问题。

变量初始化

关于一般编码实践。 从不声明未初始化的指针。理想情况下，永远不要声明未初始化的任何东西。像这样的事情:

StockNodePtr head, new, current, previous, nextStock;
unsigned listSize;

可以回来真正咬你的屁股。这些应该是这样的:

StockNodePtr head=NULL, new=NULL, currentNULL, previousNULL, nextStockNULL;
unsigned listSize = 0;

如果您担心初始化一个值只是为了在声明后立即将其重写，请不要担心。编译器会为你优化它(volatile not withstanding)。

链表构建

构造尾追加链表的一个常见问题是知道最后一个“下一个”指针在哪里。人们经常混淆两个或三个指针，特殊情况是初始构造时的头指针等。我在这里告诉你这些都不需要。

考虑一下 dbl-pointer(一个保存另一个指针变量地址的指针)如何非常有效地帮助构造期间的尾部追加列表:

StockNodePtr head = NULL;
StockNodePtr *next = &head; // points to the next pointer to assign.
while(not finished)
{
    StockNodePtr newNode = malloc(sizeof(*newNode));

    // ...
    // set all your fields.
    //

    // whatever pointer `next` points to gets the new node. on an
    //  initial list it will be the `head` pointer. on a subsequent
    //  node it will be the `nextStock` pointer of the last-node-added.
    *next = newNode;

    // now just set the new next-pointer-to-populate to be the `nextStock`
    //  pointer of the node we just added.
    next = &newNode->nextStock;
}

// terminate the last node
*next = NULL;

前面代码中的next 指针始终保存着下一个要填充的指针的地址。最初它填充有 head 指针变量的地址。当循环结束时，next 指向的任何指针都需要设置为 NULL 以终止列表。注意:设置字段时不需要设置 newNode->nextStock = NULL;。循环的下一次迭代将为您设置它(到下一个节点)或 *next = NULL; 如果它是最后添加的节点，则循环后将其设置为 NULL。

---

我会深入研究 strtok() 的用法和所有这些指针，但我晚饭晚了，我的另一半正在打电话。我希望这至少有所帮助。祝你好运，祝你有个炸药日。