-4

Hey i am starting to work on Huffman coding and I have a bit of a problem I getting this error

Segmentation fault (core dumped)

I understand it is caused by trying to reach memory you are not allow to but I can not realize what is the problem in my code, thank in advnace for the help!

src.txt - http://pastebin.com/kDf8nEhV

#include <stdio.h>
#include <stdlib.h>

int freq[256] = {0};

struct Node {
    unsigned char m_ch;
    int m_freq;
    struct Node* m_ls, *m_rs;
    struct Node* m_hls, *m_hrs;
};

struct Node* createNode(int freq, char ch);
void insertTree(struct Node** root, struct Node* n);
struct Node* getBinTree(FILE* fsrc);
void inorder(struct Node* root);

int main() {
    FILE* fsrc;
    struct Node* tree = (struct Node*)malloc(sizeof(struct Node));
    fsrc = fopen("src.txt", "rb");
    tree = getBinTree(fsrc);
    inorder(tree);
    return 1;
}

struct Node* createNode(int freq, char ch) {
    struct Node* pNode = (struct Node*)malloc(sizeof(struct Node));
    pNode->m_freq = freq;
    pNode->m_ch = ch;
    return pNode;
}

void insertTree(struct Node** root, struct Node* n) {
    if (!(*root)) {
        *root = n;
        return;
    }
    if (n->m_freq < (*root)->m_freq) {
        insertTree(&(*root)->m_ls, n);
    } else {
        insertTree(&(*root)->m_rs, n);
    }
}

struct Node* getBinTree(FILE* fsrc) {
    struct Node* temp = (struct Node*)malloc(sizeof(struct Node));
    struct Node** root = (struct Node**)malloc(sizeof(struct Node*));
    *root = (struct Node*)malloc(sizeof(struct Node));
    int c, i;
    while ((c = fgetc(fsrc)) != EOF) {
        freq[c]++;
    }
    freq[255] = 1;
    fclose(fsrc);
    for (i = 0; i < 256; i++) {
        if (freq[i] > 0) {
            temp = createNode(freq[i], i);
            insertTree(root, temp);
        }
    }
}

void inorder(struct Node* root) {
    if (root != NULL) {
        inorder(root->m_ls);
        printf(" %d\n", root->m_freq);
        inorder(root->m_rs);
    }
    return;
}
Raz
  • 1
  • 2
  • 3
    There are issues all throughout your code. Some functions aren't returning values that should be, returned values aren't being checked for errors, etc. I see many places where you could get a segfault. Which one are you wanting help with? What line are you experiencing your problem on? – Jeremy Huddleston Sequoia Apr 29 '15 at 18:22

1 Answers1

0

Your,

struct Node* getBinTree(FILE *fsrc)

is not returning anything.

You should enable compiler warnings, the compiler would have told you about that, the function is not returning a value, but you still assign it to tree in main().

But apart from that there is a serious problem with your code, a simple example

int main()
{
  FILE *fsrc;
  struct Node *tree = (struct Node*)malloc (sizeof(struct Node));
  fsrc = fopen("src.txt","rb");
  tree=getBinTree(fsrc);
  inorder(tree);
  return 1;
}

This single function has many problems,

  1. You allocate new memory and point to it with tree, but you never use it because you overwrite the pointer here

    tree = getBinTree(fsrc);
    

    that causes a memory leak.

  2. You don't check if the file was succesfuly opened, you must check

    fsrc = fopen("src.txt", "rb");
    if (fsrc == NULL)
        return -1;
    
  3. You don't need to cast malloc()

    struct Node *tree = malloc(sizeof(*tree));
    

    this is more robust, and easier to read.

  4. You have to make sure that malloc() did not fail to allocate memory, when it does NULL is returned, so you need this when you do a malloc() that is actually required, for example in getBinTree()

    struct Node *tree = malloc(sizeof(*tree)); if (tree == NULL) return NULL;

    i.e., copy malloc()'s behavior by returning NULL on failure.

Community
  • 1
  • 1
Iharob Al Asimi
  • 52,653
  • 6
  • 59
  • 97
  • http://pastebin.com/1v5xQBQ9 did those changes according to your answer , still same problem your answers (1) and (3) kinda confused me though – Raz Apr 29 '15 at 18:35
  • @Raz You improved it a lot, some style issues though but they are a matter of taste and experience, for example single line `malloc()` and error check, might be confusing, I'll take a look to see what might be happening. – Iharob Al Asimi Apr 29 '15 at 18:46
  • @Raz Don't close the file in a function that didn't open it, it could be confusing. Can you add to your question the file content please. – Iharob Al Asimi Apr 29 '15 at 18:48
  • did , and so should i not close it at all or should i colse it in the main? – Raz Apr 29 '15 at 19:08
  • You should close it where you opened it, what are you using to write the program? – Iharob Al Asimi Apr 29 '15 at 19:11
  • sublime text 2 , compling with the treminal useing linux gcc – Raz Apr 29 '15 at 19:13
  • Excelent, I recommend running the program with `gdb`, compile it with `gcc -Wall -Werror -g3 -O0` and also use [valgrind](http://www.valgrind.org). That sould help you find the problematic line. – Iharob Al Asimi Apr 29 '15 at 19:18
  • http://pastebin.com/1LXJyJdA did minor changes , compiled wtih gcc -Wall -Werror -g3 -O0 and and it gives me this error zip.c:22:16: error: variable ‘tree’ set but not used [-Werror=unused-but-set-variable] struct Node *tree; still not running and gives me the same error : Segmentation fault (core dumped) – Raz Apr 30 '15 at 12:30
  • @Raz You need to use **gdb** to check where the Segmentation Fault happens, there is no easy way to tell what's wrong. Now I have a lot of work to do, but later I might be able to take a look at your code. After you have removed the segmentation fault, check the program with valgrind and verify that there are no other silent memory errors that could trigger strange behavior under certain situations. – Iharob Al Asimi Apr 30 '15 at 12:38
  • @iharbo , http://pastebin.com/NJCxJuZ6 runned with gbd and the problem is in the method isertTree ,in if(!(*root)) it just crash beforehand – Raz Apr 30 '15 at 12:52
  • @Raz Change it to `if ((root == NULL) || (*root == NULL))`. Due to short-circuit, the second condition will not be evaluated if the first is true, and it appears that you are trying to dereference a `NULL` pointer there. – Iharob Al Asimi Apr 30 '15 at 15:05