Seg fault on binary search tree - big sorting

Question

Trying to do this Big Sorting problem on Hackerrank. I know there are simpler ways to write the algorithm, but I am trying to brush up on my C knowledge, and as such wanted to challenge myself and write a Binary Search Tree for the problem.

The binary search tree I wrote seems to work for integers, however, the problem has an input type of strings in the form of numbers. So I tried changing it to using character arrays, and strcmp() in order to compare the strings of numbers.

Can someone help me figure out what's going on here, and how I might be able to fix the issue?

Seg-Fault:

GDB trace:
Reading symbols from solution...done.
[New LWP 11287]
Core was generated by `solution'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x00007f4f76b2e455 in __strcpy_chk (dest=0xfb0020 "", src=0x0, 
    destlen=105) at strcpy_chk.c:28
#2  0x00000000004007e3 in strcpy (__src=0x0, __dest=<optimized out>)
    at /usr/include/x86_64-linux-gnu/bits/string3.h:110
#3  create_node (key=0x0) at solution.c:25
#4  0x00000000004006af in main () at solution.c:70

Code:

#include <math.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <assert.h>
#include <limits.h>
#include <stdbool.h>

#define MAXLEN 105

struct tree_node {
    char key[MAXLEN];
    struct tree_node *left;
    struct tree_node *right;
}; typedef struct tree_node Node;

Node *create_node(char *key) {
    Node *new_node;

    if ((new_node = (Node*)malloc(sizeof(Node))) == NULL) {
        printf("Cannot allocate memory for new node");
        exit(1);
    }

    strcpy(new_node->key, key);

    //new_node->key = key;
    new_node->left = NULL;
    new_node->right = NULL;

    return (new_node);
}

Node *insertNode(Node *node, char *keyInput) {
    if (node == NULL) {
        return (create_node(keyInput));
    } else {
        if (strcmp(keyInput, node->key) <= 0) {
            node->left = insertNode(node->left, keyInput);
        } else {
            node->right = insertNode(node->right, keyInput);
        }
        return (node);
    }
    return 0;
}

void printTree(Node *tree) {
    if (tree != NULL) {
        printTree(tree->left);
        printf("%s\n", tree->key);
        printTree(tree->right);
    }
}

int main() {
    // Enter your code here. Read input from STDIN. Print output to STDOUT
    int i, n = 0;
    char *input;
    Node *tree = NULL;

    scanf("%d", &n);

    for (i = 0; i < n; i++) {
        scanf("%s", input);
        tree = insertNode(tree, input);
    }

    printTree(tree);

    return 0;
}

chqrlie · Accepted Answer · 2017-09-16T19:24:51.393

1

You pass an uninitialized pointer to scanf() to read a string. You should instead define input as an array:

int main(void) {
    // Enter your code here. Read input from STDIN. Print output to STDOUT
    int i, n;
    char input[105];
    Node *tree = NULL;

    if (scanf("%d", &n) == 1) {
        for (i = 0; i < n; i++) {
            if (scanf("%104s", input) != 1)
                break;
            tree = insertNode(tree, input);
        }
        printTree(tree);
    }
    return 0;
}

The problem with this approach is it does not fit the problem space:

The website says the lines contain numbers encoded in decimal without leading zeroes, but up to 10⁶ bytes long. You cannot parse such input portably and efficiently with `scanf().
Furthermore, strcmp() is inappropriate to compare these entries: you should first compare the lengths and only use strcmp() on entries with the same number of digits.

edited Sep 16 '17 at 19:24

answered Sep 16 '17 at 18:34

chqrlie

131,814
10
121
189

Got it, that makes sense. And the `%104s` - is that necessary, or just added precaution we are not taking too much memory? – Adjit Sep 16 '17 at 18:37
Hardcoding 104 kind of defeats the purpose of using a MAXLEN define. – Bernhard Barker Sep 16 '17 at 18:44
@Dukeling: yes, `scanf()` is so clunky I should do away with the define. – chqrlie Sep 16 '17 at 19:10

gsamaras · Answer 2 · 2017-09-18T05:16:36.383

Focus here:

char *input;
for (i = 0; i < n; i++) {
    scanf("%s", input);
}

Where will be the read string going to be stored? input is char pointer that doesn't point anywhere near valid memory space!

Let's allocate some memory for it by defining it as an array:

char input[MAXLEN];
for (i = 0; i < n; i++) {
    scanf("104%s", input);
}

Do not forget the space needed by the null-terminator, that's why I used 104 (MAXLEN - 1), which is a precaution in order not to take more memory than input can store.

Moreover, you could take advantage of what scanf() returns and do this:

for (i = 0; i < n; i++) {
    if( scanf("%104s", input) != 1 )
        break; // didn't read a one string, break the loop!
    // I got the input, let's insert it to the tree..
}

Got it, yeah that makes sense. A bit rusty, so that's why I'm working on this. Thanks! — Adjit, Sep 16 '17 at 18:38

Seg fault on binary search tree - big sorting

2 Answers2