1

I'm in the second semester of my college so I just started coding. Recently we learned something about binary trees. So I just wanted to code my own ones. I decided to code a binary tree contact book.

First I'm saving my struct into a .txt file. (I want it in a txt file, not a binary, because then i can read it after the program.) After that I try to load it again in a node to rebuild my binary tree.

Here we go with a shortened version. I commented the important parts.

#define CLEN 100

struct binarytree {
    struct binarytree *left;
    struct binarytree *right;
    char *firstname;
    char *lastname;
    char *city;
    char *street;
    char *addr;
    char *tel;
} typedef btree;

//-----------------------------------------

btree *creatnullnode(void);
btree *loadtree(char *filename);

//-----------------------------------------

btree *creatnullnode(void) {
    btree *node = malloc(sizeof(btree));
    node->left  = NULL;
    node->right = NULL;

    //TODO: the memmory is not right allocated..
    node->firstname = (char*)malloc(CLEN * sizeof(char));
    node->lastname  = (char*)malloc(CLEN * sizeof(char));
    node->city      = (char*)malloc(CLEN * sizeof(char)); 
    node->street    = (char*)malloc(CLEN * sizeof(char));
    node->addr      = (char*)malloc(CLEN * sizeof(char));
    node->tel       = (char*)malloc(CLEN * sizeof(char));
    return node;
}

btree *loadtree(char *filename) {
    FILE *fp;
    btree *tree = NULL;
    btree *node = creatnullnode();
    char ch = "";
    int lines = 0;

    fp = fopen(filename,"r");
    if (!fp) {
        printf("Error. no file\n");
        return NULL;
    } else {
        while (!feof(fp)) {
            ch = fgetc(fp);
            if (ch == '\n')
                lines++;
        }
        fseek(fp, 0,(int)lines % 2);

        //TODO: right here the memory of every char can't be read anymore
        fscanf(fp, "%s\t\t%s\t\t\t%s\t%s\t\t%s\t\t%s\n",
               &node->firstname, &node->lastname, &node->addr, &node->city, 
               &node->street, &node->tel);

        tree = insertnode(tree, node);

        fseek(fp, 0, 0);
        //rekursiveload(&tree, fp);      //TODO: - ausprogrammieren -
    }

    fclose(fp);
    return tree;
}

While debugging I saw that the memory did not get correctly allocated. But i don't know how to fix it.

after allocationg the char[] is set to: node->firstname = 0x007db250 "ÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍÍýýýýM¸Þµ¦æ" Debugger says: <Error reading the characters of the string.> after fscanf

chqrlie
  • 131,814
  • 10
  • 121
  • 189
Pleasure
  • 23
  • 5
  • If you're always allocate the exact same constant size for the strings, why not create them as *arrays*? Like e.g. `char firstname[CLEN];`? – Some programmer dude Mar 30 '19 at 12:34
  • 1
    By the way, your code as you show it should not even build, much less run and give wrong results. – Some programmer dude Mar 30 '19 at 12:35
  • 1
    Lastly, what do you mean by "that the memory did not get correctly allocated"? How did you see it? What happens? What should happen? Please read about [how to ask good questions](http://stackoverflow.com/help/how-to-ask), as well as [this question checklist](https://codeblog.jonskeet.uk/2012/11/24/stack-overflow-question-checklist/). – Some programmer dude Mar 30 '19 at 12:37
  • 1
    Oh I have a hint for you: When using `fscanf` (and family) with the `"%s"` specifier, it expects a pointer to the first element of the destination string, of type `char *`. Now think a little, what is the type of e.g. `&node->firstname`? – Some programmer dude Mar 30 '19 at 12:39
  • 1
    https://ericlippert.com/2014/03/05/how-to-debug-small-programs/ – melpomene Mar 30 '19 at 13:05

1 Answers1

3

There are several issues in your code:

  • while (!feof(fp)) is always wrong for testing end of file: Why is “while (!feof(file))” always wrong? You should instead write this:

        while ((ch = fgetc(fp)) != EOF) {
            ...
    
  • you should create a new node for each line that you read from the file. Currently you reuse the same memory for each node and overwrite the fields with the new data. insertnode, which is missing from the code fragment, most probably creates a loop in the list, causing undefined behavior when you try and free it.

  • char ch = ""; is incorrect: "" is a string, not a char, and ch must be defined as an int to read bytes with fgetc() and store EOF too.

  • fseek(fp, 0,(int)lines % 2); is meaningless. What are you trying to achieve? you can try and rewind the stream with rewind(fp) or fseek(fp, 0L, SEEK_SET), but you will only be able to read a single line .

  • fscanf(fp, "%s\t\t%s\t\t\t%s\t%s\t\t%s\t\t%s\n", &node->firstname, ... has multiple issues: you cannot prevent incorrect input from causing too many characters to be stored into the destination arrays, and you should pass pointers to the destination arrays, not addresses of pointers. In other words, the code should be:

        char eol;
        if (fscanf(fp, "%99s%99s%99s%99s%99s%99s%c",
            node->firstname, node->lastname, node->addr, 
            node->city, node->street, node->tel, &eol) != 7 || eol != '\n') {
            /* invalid input... */
        }
    

    A much safer approach to read this input is to read a single line into a larger array of char and use sscanf() to parse this line into the node fields... but looking at your format string, it seems you are dealing with TAB separated values and

  • Neither fscanf(), nor sscanf() nor even strtok() can properly parse TAB separated values from a text file. You need to write you own code for this. I suggest you use strcspn() to compute the field lengths and strndup() to allocate strings from a range in a char array.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • 1
    And it should be noted that there are so many more issues with the code. Including *undefined behavior* (which seems to be the reason behind the question). – Some programmer dude Mar 30 '19 at 12:51
  • @Someprogrammerdude yep you're right. The code is maybe full of errors. But thats why I'm here. The other parts of it working fine... more or less... pointers cool somehow. I wanted to use themto practice. I think to say at the beginning char firstname[CLEN] could be easyer – Pleasure Mar 30 '19 at 13:02
  • @Pleasure: I am doing exactly what you expect: point out errors, some minor, some fatal and provide answers on how to fix them. The allocation seems correct, although I would define the structure for fixed arrays such as `char firstname[CLEN];` to avoid a separate allocation for each field and you should check for allocation failure everywhere. – chqrlie Mar 30 '19 at 13:03
  • @chqrlie waiiiit. I'm not as fast as you. thaaaaaaaank you !!! :) ok I try that. I appreciate your answer .. and improvements a lot .. and accept everything constructively – Pleasure Mar 30 '19 at 13:06
  • same to @Someprogrammerdude – Pleasure Mar 30 '19 at 13:06