Confusion with pointers in a deleting bst function

Question

So this is my code. It should delete a sub-tree of a BST, "buscar" function is the search function, then it passes to "eliminar" the correct pointer and then I got confused with pointers. Should I free(*arbol) or free(arbol)?

struct s_nodo
{
    struct s_nodo * der;
    struct s_nodo * izq;
    int valor;
};
typedef struct s_nodo * t_nodo;

void buscar (t_nodo * arbol,int num)
{
    if(*arbol != NULL)
    {
        if((*arbol)->valor == num)
            eliminar(arbol);
        else
        {
            if((*arbol)->valor > num)
                buscar(&(*arbol)->izq,num);
            else if((*arbol)->valor < num)
                buscar(&(*arbol)->der,num);
        }
    }
    else
    {
        return;
    }    
}

void eliminar (t_nodo * arbol)
{
    if(*arbol != NULL)
    {
        eliminar(&(*arbol)->izq);
        eliminar(&(*arbol)->der);
        free(*arbol); //problematic line
        *arbol = NULL;
    }
}

Answer 1

Your code is fine with free(*arbol). An SSCCE (Short, Self-Contained, Correct Example) demonstrates this:

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

struct s_nodo
{
    struct s_nodo *der;
    struct s_nodo *izq;
    int valor;
};
typedef struct s_nodo *t_nodo;

static void eliminar(t_nodo *arbol)
{
    if (*arbol != NULL)
    {
        eliminar(&(*arbol)->izq);
        eliminar(&(*arbol)->der);
        free(*arbol); //problematic line
        *arbol = NULL;
    }
}

static void buscar(t_nodo *arbol, int num)
{
    if (*arbol != NULL)
    {
        if ((*arbol)->valor == num)
            eliminar(arbol);
        else
        {
            if ((*arbol)->valor > num)
                buscar(&(*arbol)->izq, num);
            else if ((*arbol)->valor < num)
                buscar(&(*arbol)->der, num);
        }
    }
}

static void p_inorder(t_nodo node)
{
    if (node != 0)
    {
        p_inorder(node->izq);
        printf(" %d", node->valor);
        p_inorder(node->der);
    }
}

static void print(char *tag, t_nodo root)
{
    printf("%s: ", tag);
    p_inorder(root);
    putchar('\n');
}

int main(void)
{
    t_nodo left = malloc(sizeof(*left));
    left->valor = 3;
    left->izq = 0;
    left->der = 0;
    t_nodo right = malloc(sizeof(*right));
    right->valor = 6;
    right->izq = 0;
    right->der = 0;
    t_nodo root = malloc(sizeof(*root));
    root->valor = 5; 
    root->izq = left;
    root->der = right;

    print("Before", root);
    buscar(&root, 3);
    print("After", root);
    free(right);
    free(root);
    return 0;
}

When run on Ubuntu 13.10 with valgrind, you get a clean bill of health.

==6217== Memcheck, a memory error detector
==6217== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==6217== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==6217== Command: ./bst3
==6217== 
Before:  3 5 6
After:  5 6
==6217== 
==6217== HEAP SUMMARY:
==6217==     in use at exit: 0 bytes in 0 blocks
==6217==   total heap usage: 3 allocs, 3 frees, 72 bytes allocated
==6217== 
==6217== All heap blocks were freed -- no leaks are possible
==6217== 
==6217== For counts of detected and suppressed errors, rerun with: -v
==6217== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

I also ran a variant of the program which deleted value 5 (and did not explicitly free right or root); the 'After' tree was empty and valgrind was equally happy.

---

I also completely agree with the comment by WhozCraig that embedding a pointer in the typedef for a non-opaque type is apt to be confusing — I would have written:

typedef struct Node Node;

since the structure tag name space is separate from the ordinary identifiers name space, and would never write:

typedef struct Node *pNode;

Think of FILE in <stdio.h>; you always use FILE * everywhere; I'd use Node * everywhere I passed a pointer to a Node, and Node ** when relevant.

See also typedef pointers — a good idea?

---

What happens with free(arbol) because that works on my PC?

`valgrind` gets justifiably upset:

==6284== Memcheck, a memory error detector
==6284== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==6284== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==6284== Command: ./bst3
==6284== 
Before:  3 5 6
==6284== Invalid free() / delete / delete[] / realloc()
==6284==    at 0x4C2B60C: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x400C23: eliminar (bst3.c:18)
==6284==    by 0x400636: main (bst3.c:28)
==6284==  Address 0x51fc108 is 8 bytes inside a block of size 24 alloc'd
==6284==    at 0x4C2A2DB: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x4005D5: main (bst3.c:66)
==6284== 
==6284== Invalid write of size 8
==6284==    at 0x4011BB: eliminar (bst3.c:19)
==6284==    by 0x400636: main (bst3.c:28)
==6284==  Address 0x51fc100 is 0 bytes inside a block of size 24 free'd
==6284==    at 0x4C2B60C: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x4011BA: eliminar (bst3.c:18)
==6284==    by 0x400636: main (bst3.c:28)
==6284== 
==6284== Invalid free() / delete / delete[] / realloc()
==6284==    at 0x4C2B60C: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x4011CA: eliminar (bst3.c:18)
==6284==    by 0x400636: main (bst3.c:28)
==6284==  Address 0x7fefffda0 is on thread 1's stack
==6284== 
After: 
==6284== 
==6284== HEAP SUMMARY:
==6284==     in use at exit: 48 bytes in 2 blocks
==6284==   total heap usage: 3 allocs, 3 frees, 72 bytes allocated
==6284== 
==6284== LEAK SUMMARY:
==6284==    definitely lost: 48 bytes in 2 blocks
==6284==    indirectly lost: 0 bytes in 0 blocks
==6284==      possibly lost: 0 bytes in 0 blocks
==6284==    still reachable: 0 bytes in 0 blocks
==6284==         suppressed: 0 bytes in 0 blocks
==6284== Rerun with --leak-check=full to see details of leaked memory
==6284== 
==6284== For counts of detected and suppressed errors, rerun with: -v
==6284== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 2 from 2)

This was for the second case I tested: the tail of the main() was:

    print("Before", root);
    buscar(&root, 5);
    print("After", root);
    return 0;
}