0
\$\begingroup\$

I've been studying more C using C Programming: A Modern Approach: A Modern Approach 2nd Edition by K.N. King as well as reading the comp.lang.c FAQ and the Code Review section here on Stack Exchange to improve my C skills. I would like your feedback on this recent program I wrote to calculate the volume of a box. It uses ncurses, currently just featuring text input/output but I am planning on expanding on that as I learn more about how to use ncurses.

Specifically, I would like to know thoughts on the following:

  1. I am currently using an approach of getnstr(*str, n) → temporary buffer → conversion to uint64_t. I have implemented this to prevent buffer overflow attacks. Is my approach a good way to check this? From what I've read, I saw that scanf() family of functions isn't the best choice for scanning input, so I have used advice I previously received here to implement a solution.

  2. Is my approach to "error handling" a good one? Besides gracefully asking again for input upon failure (I know that would be a better approach), is this errno assignment a good practice, or is there a preferred way to do this in C? I saw a few examples online when I searched this, most people used something similar yet others used raise and some used longjmp/setjmp. I know there may be multiple right ways of doing this, but what would be preferred?

  3. my_strto64 is implemented for conversion to uint64_t. Is this function safe? Can it be improved in any way? This function is a slightly modified version of one I received as feedback from previous code which I had received as an answer to a previous Code Review on here.

  4. Currently I'm just checking the return value of sprintf - I would like to have some way to indicate to the user if the supplied values are not outputting the correct result due to the handling of overflow during multiplication. I know this is quite a simple program as it is, but this is something I was thinking about and I did think of checking the length of each string but couldn't think of a clean way to check proactively on this scenario as the lengths of individual strings aren't enough to ensure the right result after multiplication. It is something I definitely want to learn. I would appreciate any advice on this. I was thinking maybe something like dividing the maximum possible uint64_t value by one of the numbers and then multiplying the other two numbers and comparing results to see the possiblity of overflow.

  5. Should calloc/malloc be used, or a stack-based allocation such as malloca? I have seen some people say it's better to use malloca or an array-based creation for strings on the stack instead of allocating memory on the heap, due to stack memory automatically freeing at the end of a function. Which is a better choice in this scenario?

Would appreciate your feedback on the points above and/or anything else you find that can be improved in my code. I always learn a lot from all of you knowledgeable folks here.

#include <stdlib.h>
#include <ncurses.h>
#include <stdint.h>
#include <inttypes.h>
#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <ctype.h>
#include <limits.h>

#define DECSTR_UINT64_MAX_LENGTH 20

int my_strto64(uint64_t *dest, const char *input_str);

int main(void) {
    char tmp_str[DECSTR_UINT64_MAX_LENGTH + 1]; // for input prior to parsing (+1 for NUL)
    uint64_t height, width, length;
    // length + 1 (NUL character)
    char* total_string = calloc(DECSTR_UINT64_MAX_LENGTH + 1, 1);

    if (!total_string) {
        errno = ENOMEM;
        perror("Unable to allocate memory");
        return errno;
    }

    initscr();
    printw("--- Volume Calculator --\n");
    printw("Enter length: ");
    getnstr(tmp_str, DECSTR_UINT64_MAX_LENGTH);
    if(!my_strto64(&length, tmp_str)) {
        errno = EIO;
        perror("Unable to scan length");
        return errno;
    }

    printw("Enter width: ");
    getnstr(tmp_str, DECSTR_UINT64_MAX_LENGTH);
    if(!my_strto64(&width, tmp_str)) {
        errno = EIO;
        perror("Unable to scan length");
        return errno;
    }

    printw("Enter height: ");
    getnstr(tmp_str, DECSTR_UINT64_MAX_LENGTH);
    if(!my_strto64(&height, tmp_str)) {
        errno = EIO;
        perror("Unable to scan length");
        return errno;
    }

    int return_value = sprintf(total_string, "Total: %" PRIu64, height * length * width);
    // sprintf returns a negative value if it fails, so check it
    if (return_value < 0) {
        errno = EIO;
        perror("Cannot multiply height * length * width");
        return errno;
    }
    printw(total_string);

    free(total_string);

    refresh();
    getch();
    endwin();
    return 0;
}

/**
 * Converts input_str to uint64_t -> returns 0 on success
 * Adapted from: https://codereview.stackexchange.com/a/206773/78786
 */
int my_strto64(uint64_t *dest, const char *input_str) {
  char *endptr;
  errno = 0;
  unsigned long long parsed_long_long = strtoull(input_str, &endptr, 10);
  #if ULLONG_MAX > UINT64_MAX
  if (y > UINT64_MAX) {
    uint64_t *dest = UINT64_MAX;
    errno = ERANGE;
    return errno;
  }
  #endif
  *dest = (uint64_t) parsed_long_long;
  if (errno == ERANGE) {
    return errno;
  }

  // `strtou...()` function wraps with `-`
  // lets return an error if its negative
  if (*dest && strchr(input_str, '-')) {
    *dest = 0;
    errno = ERANGE;
    return errno; // negative, we don't want it
  }

  if (input_str == endptr) {
    errno = EDOM;
    return errno; // unsuccessful at converting, still *char
  }

  while (isspace((unsigned char) *endptr)) endptr++;

  if (*endptr) {
    errno = EIO;
    return errno; // contains invalid characters
  }

  return (int) parsed_long_long;
}
\$\endgroup\$
3
\$\begingroup\$

Error handling

unsigned long long is guaranteed to hold at least 64 bits. If you are using strtoull(), then limiting yourself to uint64_t is just asking for trouble with no benefit. In fact, your conditional code in #if ULLONG_MAX > UINT64_MAX doesn't even compile: there is no such variable y.

What does the return value of my_strto64() represent? Sometimes, it's an error code. But if there was no error, then it's the parsed_long_long‽ And the parsed_long_long is cast as an int for some reason? That doesn't make sense at all.

In the main() function, if any of the calls to my_strto64() fails, then you terminate the program without calling endwin(), leaving the terminal in a bad state.

Realistically, sprintf() is not going to fail in a way that would result in a negative return value. What could possibly go wrong with writing some string to a buffer that has already been allocated? If it's buffer overflow — and you do have a buffer overflow problem, because your total_string doesn't have enough space to contain "Total: " —, then it's likely to either segfault or fail silently. (To guard against the segfault, you could use snprintf(), but a full buffer would result in a positive, not negative, return value.) If it's integer overflow from the multiplication, then it won't detect it either, since the multiplication is simply done modulo 264. (Unlike sprintf(), printf() might fail, if it tries to write to STDOUT and it is closed. I suppose that printw() could fail too, but you never check for those errors — and I wouldn't bother either.)

Miscellaneous

Labelling the output as a "total" is a bit weird to me, since it implies that it's a sum rather than a product of the inputs. (Airline luggage rules often place a limit on the length + width + height of an item, for example.)

It is customary to put main() at the end, to avoid needing to write forward declarations.

I suggest putting the #includes in alphabetical order.

The code for reading the three dimensions is repetitive. Furthermore, it looks like you have a copy-and-paste error, since all three error messages are the same. You should define a helper function.

Using calloc() to allocate a string of a short, limited length is not worth the trouble. Putting it on the stack would be fine. But I wouldn't bother with composing total_string at all — just have printw() format the string for you.

Suggested solution

#include <ctype.h>
#include <errno.h>
#include <inttypes.h>
#include <limits.h>
#include <ncurses.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>

// Generous estimate of the maximum number of digits
// https://stackoverflow.com/a/10536254
#define ULL_DIGITS (3 * sizeof(unsigned long long))

/**
 * Prints a prompt then reads an unsigned long long, using ncurses.
 * Returns 1 on success.  Returns 0 on failure, with errno set to
 * ERANGE, EDOM, or EIO.
 */
int ask_ull(unsigned long long* result, const char *prompt) {
    char buf[ULL_DIGITS + 1];
    char *endptr;
    printw("%s", prompt);
    getnstr(buf, ULL_DIGITS);
    *result = strtoull(buf, &endptr, 10);
    if (errno == ERANGE) {                      // Overflow or underflow
        return 0;
    }
    if (endptr == buf || strchr(buf, '-')) {    // Unsuccessful conversion
        errno = EDOM;
        return 0;
    }
    while (isspace(*endptr)) endptr++;
    if (*endptr) {                              // Trailing junk
        errno = EIO;
        return 0;
    }
    errno = 0;
    return 1;
}

int main(void) {
    unsigned long long height, width, length;
    char *errmsg = NULL;

    initscr();
    printw("--- Volume Calculator --\n");

    if (!errmsg && !ask_ull(&length, "Enter length: ")) {
        errmsg = "Unable to scan length";
    }
    if (!errmsg && !ask_ull(&width, "Enter width: ")) {
        errmsg = "Unable to scan width";
    }
    if (!errmsg && !ask_ull(&height, "Enter height: ")) {
        errmsg = "Unable to scan height";
    }
    if (errmsg) {
        refresh();
        endwin();
        perror(errmsg);
        return 1;
    }

    unsigned long long volume = length * width * height;
    printw("Volume: %llu", volume);

    refresh();
    getch();
    endwin();
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the in depth answer, I learned a lot. One question I have is why does the function return 0 on errors? Isn't 0 supposed to be indictive of success and any non-zero code failure,? I see you're doing if(!ask_ull), would this work if the function returns an error? Since 0 (success) would skip over the if statement wouldn't it? Is returning errno a better idea? \$\endgroup\$ – Faraz Dec 25 '18 at 13:50
  • \$\begingroup\$ Also is this conversation that I did from string to unsigned long long best practice? Or is there another method that's included in the standard library or a vetted open source library to accomplish this which I should choose instead? \$\endgroup\$ – Faraz Dec 25 '18 at 14:17

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.