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

• Faraz: 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?
• Faraz: 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?