OMG WTF OpenSSL

If you are a developer, I highly recommend you follow OpenSSL Valhalla Rampage. The latest entry refers to a file called pqueue.c. I took a look at the file … boy, that hurts. Let’s pick some highlights.

typedef struct _pqueue {
    pitem *items;
    int count;
} pqueue_s;

So we have a opaque struct that holds all data of a queue. Note the count element. It is never used. There is a function pqueue_size but that function walks through the linked list to count the elements:

int
pqueue_size(pqueue_s *pq)
{
    pitem *item = pq->items;
    int count = 0;

    while (item != NULL) {
        count++;
        item = item->next;
    }
    return count;
}

To get a new item, there is a function pitem_new that takes two arguments: The data to store and a pointer to an unsigned char array. As it is obvious from the argument’s name prio64be this needs to be an array of 8 bytes that represent the priority of the item, stored as big endian.

pitem *
pitem_new(unsigned char *prio64be, void *data)

Why big endian, one might ask. Well, it turns out that pqueue_insert uses memcmp to sort the items by priority:

/* we can compare 64-bit value in big-endian encoding
 * with memcmp:-) */
int cmp = memcmp(next->priority, item->priority,
         sizeof(item->priority));
if (cmp > 0)        /* next > item */
{

Of course, you could use uint64_t for the priority and simply compare that values …

Let’s pick another function:

pqueue_s *
pqueue_new(void)
{
    pqueue_s *pq = (pqueue_s *)malloc(sizeof(pqueue_s));

    if (pq == NULL)
        return NULL;

    memset(pq, 0x00, sizeof(pqueue_s));
    return pq;
}

Could you simplify this method? How about:

pqueue_s *
pqueue_new(void)
{
    return (pqueue_s *)calloc(1, sizeof(pqueue_s));
}

Free could be a bit easier, too. Replace:

void
pqueue_free(pqueue_s *pq)
{
    if (pq == NULL)
        return;

    free(pq);
}

… with …

void
pqueue_free(pqueue_s *pq)
{
    free(pq);
}

In pqueue_find you find:

pitem *found = NULL;

[…]

if (!found)
    return NULL;

return found;

And so on. With one or maybe two exception, every single function in the file has (serious) flaws. Besides the copyright, there are 3 lines of documentation (two of them shown above). The third one is:

if (item == NULL || *item == NULL)
    return NULL;

/* *item != NULL */
ret = *item;

I truly hope that the LibreSSL guys have enough patience to fix all the mess OpenSSL is.