Number converter between bases

Question

I just wrote a utility program in C that converts numbers of different bases to 'standard' well-known bases (hex, dec, oct, bin). I called it repnum, short for "represent number."

You will notice a 'template' that I like to employ when writing C programs. Any suggestions?

repnum.h

#ifndef REPNUM_H
#define REPNUM_H

#ifdef __GNUC__
#define _GNU_SOURCE
#elif ! (_POSIX_C_SOURCE >= 2 || _XOPEN_SOURCE)
#define NO_OPTS
#endif

#define BUF_SIZE (1024)
#define PROG_VERSION (1.0f)
#define PROG_NAME "repnum"

#define iseven(n) (n % 2 == 0)
#define isodd(n) (n % 2 != 0)

#define _reporterror(c, ...) \
    err(c, __VA_ARGS__)
#define _throwerror(c, ...) \
    errx(c, __VA_ARGS__)

/* Not needed for now. Just for later use.
#ifdef NDEBUG
#define _assert(c, ...) \
    c ? 0 : _throwerror(2, __VA_ARGS__)
#else
#include <assert.h>
#define _assert(c, ...) \
    assert(c)
#endif
*/

/*
 * Converts a number to a string containing its binary representation.
 * `num` is the number to convert.
 * `buf` is the string buffer to use.
 * `n` is the length of the string.
 * Returns a pointer to the start of the representation. NOT THE START OF `buf`.
 * If NULL was returned, then `n` characters are not enough and the function stopped at the middle of processing.
 * NOTE: the algorithm used writes binary right to left, so to ensure highest performance,
 * characters are written starting by the end of the string.
 */
char *num2bin(unsigned long long, char *, size_t);

/*
 * Converts a string to a long.
 * Converts `str` as a base `base` number to long and stores it in `num`.
 * Supports all bases that strtoull supports.
 * Returns:
 *  0 - Whole `str` is valid. `num` is changed.
 *  1 - Whole `str` is invalid. `num` is not changed.
 *  2 - `num` is null or `str` is null or the first character in `str` is a null byte. `num` is not changed.
 *  3 - Partial `str` is valid. `num` is changed.
 *  4 - Overflow. `num` is not changed.
 */
int str2ull(char *, unsigned long long *, int);

/*                  Convenience Functions                       */
static inline unsigned long long ensurenum(char *, unsigned long long *, int);
static inline int ensurebase(int);
static inline void _free(void);
static inline void help(int);
static inline unsigned long long countbits(unsigned long long value);

#ifndef NO_OPTS
static void parseopts(int argc, char **argv);
#endif

#endif /* REPNUM_H */

repnum.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <err.h>
#include <errno.h>
#include "repnum.h"

#ifndef NO_OPTS
#ifdef _GNU_SOURCE
#include <getopt.h>
#else
#include <unistd.h>
#endif
#endif

char *_buffer;

struct numinfo {
    unsigned long long num;
    int base;
};

struct numinfo info;

int main(int argc, char **argv)
{
    char *res;

    info.base = 0;
    parseopts(argc, argv);

    if (!(_buffer = calloc(BUF_SIZE, sizeof(char))))
        _reporterror(1, "Cannot allocate memory");

    atexit(_free);

    if (!(res = num2bin(info.num, _buffer, BUF_SIZE)))
        _throwerror(1, "%s is a too large number.", argv[1]);

    printf("[dec]\t%llu\t=\t[hex]\t%llx\t[oct]\t%llo\t[bin]\t%s\n", info.num, info.num, info.num, res);

    return 0;
}

char *num2bin(unsigned long long num, char *buf, size_t n)
{
    char s;

    buf[--n] = 0;

    while (--n && num) {
        s = iseven(num) ? '0' : '1';
        buf[n-1] = s;
        num /= 2;
    }

    return &buf[n];
}

int str2ull(char *str, unsigned long long *num, int base)
{
    char *end;
    int status;

    if (!str || !*str || !num)
        return 2;

    if(!(status = strtoull(str, &end, base)) && (end == str))
        return 1;

    if (errno == ERANGE)
        return 4;

    *num = status;

    return (*end ? 3 : 0);
}

static inline void _free()
{
    free(_buffer);
}

static inline void help(int c)
{
    puts("Displays a number in various base representations.\n\t-b"
#ifdef _GNU_SOURCE
        "|--base"
#else
        "\t"
#endif
        " [base]\tforce a base. Possible values are 2 through 36.\n\t-v"
#ifdef _GNU_SOURCE
        "|--version"
#endif
        "\t\toutput version then exit.\n\t"
#ifdef _GNU_SOURCE
        "--help\t"
#else
        "-h"
#endif
        "\t\tview this help.");
    exit(c);
}

#ifndef NO_OPTS
static void parseopts(int argc, char **argv)
{
    int opt;

#ifdef _GNU_SOURCE
    struct option opts[] = {
        { "base", 1, 0, 'b' },
        { "help", 0, 0, 'h' },
        { "version", 0, 0, 'v' }
    };
#define parser() (getopt_long(argc, argv, "b:hv", opts, 0))
#else
#define parser() (getopt(argc, argv, "b:hv"))
#endif
    while ((opt = parser()) != -1) {
        switch (opt)
        {
            case '?':
                _throwerror(1, "Unrecognized option: -%c", opt);
            case 'b':
                info.base = ensurebase(ensurenum(optarg, 0, 10));
                break;
            case 'h':
                help(0);
            case 'v':
                printf("%s v%.2f. Licenced under the GNU GPL v3 License.\n", PROG_NAME, PROG_VERSION);
                exit(0);
        }
    }

    info.num = ensurenum(argv[optind], 0, info.base);
}
#endif

/*                              Convenience Functions                           */

static inline unsigned long long ensurenum(char *s, unsigned long long *n, int b)
{
    unsigned long long temp;
    int status;
    char tmp[21];

    /* if the user doesn't provide his variable two change, create a temporary one */
    if (!n)
        n = &temp;

    status = str2ull(s, n, b);

    /* generate a message to indicate required base if `b` is not 0 [any base]. */
    if (b)
        sprintf(tmp, "Base %d is required.%c", b, 0);

    switch (status)
    {
        case 1:
        case 3:
            _throwerror(1, "%s is not a valid number.%s", s, b ? tmp : "");
        case 4:
            _throwerror(1, "%s is a too large number.", s);
    }

    return *n;
}

static inline int ensurebase(int b)
{
    if (b < 2 || b > 36)
        _throwerror(1, "Unsupported base: %d", b);
    return b;
}

Answer 1

You have created a header file, but it's not clear why. Your code is not constructed as a library that others can call, but is a standalone program. A lot of the things you have put in the header file are specific to the implementation of your program. For just one example, BUF_SIZE is not relevant to any other code that might call your functions. Another example, iseven() and isodd() are internal functions that don't need to be in a header file.

Your implementations of iseven() and isodd() macros do not properly parenthesise their arguments. Instead use:

#define iseven(n) ((n) % 2 == 0)
#define isodd(n) ((n) % 2 != 0)

You would be able to see the problem if you were to call something like iseven(x+1).

What are err and errx? Those don't seem to be defined anywhere.

Your function documentation refers to argument names like num, buf, and n, but your function prototypes do not have argument names so it's unclear what parameters you are referring to.

The str2ull function would benefit from some constants defining the possible return values, or perhaps an enum.

Never define static functions in a header file. It doesn't make sense for another module to include your header file and try to reference those functions.

Some comments about what NO_OPTS is and why it is necessary would be helpful. It appears to disable large chunks of your program but the conditions under which it is enabled are unclear.

There is no need for _buffer to be global. It's only used in one function. Also, there is no need for it to be dynamically allocated, since BUF_SIZE is a constant. It can be a local variable of main:

int main(int argc, char **argv)
{
    char buffer[BUF_SIZE];
    // ...
}

Declaring it as a local variable means you can remove the stuff about checking for allocation errors, and atexit(), and _free.

It is generally considered good style to always surround blocks (such as an if statement) by { }, even if it is a single statement. This is not required by the language, and you will find people holding both opinions, but there is no disadvantage to doing so, and you will save yourself from editing or reading mistakes later.

I find the combined function-assignment-negate-test in the following unnecessarily hard to read:

if (!(res = num2bin(info.num, _buffer, BUF_SIZE)))

I would write this as:

res = num2bin(info.num, _buffer, BUF_SIZE);
if (res == NULL)

Note that I have used the explicit == NULL to check whether the res pointer is null. I find this is much easier to read than (!res), but is a matter of preference.

An advantage of using a locally allocated char buffer[BUF_SIZE] is you can use sizeof(buffer) in the above call to num2bin instead of explicitly saying BUF_SIZE. That way, you can minimise the number of things you need to change if you were to change the declaration of buffer. Also, it makes it easy to see that (..., buffer, sizeof(buffer)) matches the actual size of buffer.

I think you've got an off by one error in num2bin. Your algorithm seems to skip over the second-to-last byte in buf, not writing anything there. This will appear to work if that byte happens to contain a 0, but that's certainly not guaranteed.

Your num2bin returns an empty string if num is zero. It should return "0".

The placement of the "return" parameter in str2ull is unusual, it's in the middle position. Generally, one would design the function so the "return" parameters are at the beginning, or the end, of the parameter list depending on your preference. The middle is a strange position for that.

You have defined a number of functions as inline, but this is almost certainly not necessary for this program. And it's definitely not necessary for _free, which you take the address of anyway.

What does _GNU_SOURCE mean for your program and why does this modify the help output? Under what conditions does the behaviour of your program change?

Your ensurenum function appears to return the same information in two different ways: both in the *n parameter and as a return value. Choose one. Your code always passes 0 to n, so I would suggest using a return value only and eliminating the n parameter.

In your call to sprintf, you are unnecessarily using the %c format with a 0 parameter, perhaps in an attempt to ensure that the resulting string is nul-terminated. This is not necessary. sprintf always nul-terminates its output.

You have chosen to make tmp the exact number of bytes that your message is. Because sprintf always nul-terminates its output, this is insufficient and you have a buffer overflow. Also (if you fix the above comment about %c and therefore make it the correct length), a future modification to change the message may cause the result to be longer than what can be held in tmp. You should do two things here: (1) use snprintf to ensure that you cannot overwrite the bounds of the buffer, and (2) declare tmp to be much larger, say tmp[100].

Answer 2

Buffer overflow

char tmp[21];

if (b)
    sprintf(tmp, "Base %d is required.%c", b, 0);

If b is 10, then you will print 22 characters here. I'm not sure why you used %c to terminate the string, but it adds an extra null character to your string (the string becomes terminated with double null characters). If you had just done this, it would have been fine:

     sprintf(tmp, "Base %d is required.", b);

But rather than just make your string perfectly the right size, I suggest making it a lot larger. You never know how things might change in the future (for example, you start supporting bases past 100). Memory is cheap, and there's no difference between a 21 character buffer and a 32 (or 64) character buffer.

Magic numbers

The statuses returned by str2ull are 1, 2, 3, and 4. These are meaningless, and a year from now you won't remember what they mean either. You should use identifiers instead of numbers. For example:

enum status {
    STATUS_OK,
    STATUS_BAD_ARGUMENT,
    STATUS_EMPTY_ARGUMENT,
    STATUS_EXTRA_CHARS,
    STATUS_RANGE_ERROR,
};

switch (status)
{
    case STATUS_BAD_ARGUMENT:
    case STATUS_EXTRA_CHARS:
        _throwerror(1, "%s is not a valid number.%s", s, b ? tmp : "");
    case STATUS_RANGE_ERROR:
        _throwerror(1, "%s is a too large number.", s);
}

Precision truncated

There is a problem in str2ull() when you get the value from strtoull():

int status;

if(!(status = strtoull(str, &end, base)) && (end == str))
    return 1;

Notice that you store the unsigned long long return value of strtoull() into status, which is an int. I'm not actually sure why you need that status variable in the first place, since you never set it to an actual status. I removed status and changed that line to this:

if(!(*num = strtoull(str, &end, base)) && (end == str))
    return 1;

Strange behavior with no arguments

I ran your program with no arguments. Rather than print the help info, it gave me some random output:

[dec] 1629102960 = [hex] 611a2370 [oct]
14106421560 [bin] 1100001000110100010001101110000