Calculating the volume of a box

Question

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;
}

Answer 1

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 2⁶⁴. (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();
}