0

I just want to know if this memory that I allocated to the pointer passed to readItem() is being freed correctly in the main(), and if it´s wrong, what should I do to make it right.

Thank you for the help, and have a great day!

typedef struct Item
{
    char* itemName;
    int quantity;
    float price;
    float amount;
} Item;

void readItem(Item* ptr);

int main(int argc, char* argv[])
{
    Item sample;
    Item* p_sample = &sample;

    readItem(p_sample);
    printItem(p_sample);

    free(p_sample->itemName);
}

void readItem(Item* ptr)
{
    int size;
    printf("\n Specify the amount of letters of the product name: ");
    scanf("%d", &size);

    ptr->itemName = (char*) malloc (size * sizeof(char));

    if (ptr->itemName != NULL)
    {
        printf("\n Specify the name of the product (MAX %d letters): ", size);
        scanf("%s", ptr->itemName);

        printf("\n How many products do the company have? ");
        scanf("%d", &(ptr->quantity));

        printf("\n How expensive is the product? ");
        scanf ("%f", &(ptr->price));

        ptr->amount = (ptr->price)*(ptr->quantity);

    }
    else {
        exit(-1);
    }
}
Barmar
  • 741,623
  • 53
  • 500
  • 612
  • Found it -- yes your use of `free()` is correct -- your use of `malloc` isn't. In C, there is no need to cast the return of `malloc`, it is unnecessary. See: [Do I cast the result of malloc?](http://stackoverflow.com/q/605845/995714). Further `sizeof(char)` is defined as `1` and should be omitted. You cannot use any user-input function correctly unless you ***check the return***. – David C. Rankin Jul 30 '20 at 01:01
  • It´s unnecessary to cast the return of `malloc` because the computer is going to do automatically for me, right? – Marcellus T. Biancardi Jul 30 '20 at 02:20
  • I heard from my friends that this program is causing a memory leak. What should I change to make it right? And what is a good way of checking my return if not using a function to print out all the interesting variables? – Marcellus T. Biancardi Jul 30 '20 at 02:27
  • It is unnecessary to cast the return of `malloc` because any pointer may be converted to or from type `void*` (a `void` pointer) without a cast. `malloc` returns type `void*` (the beginning address to the block of memory allocated). Your friend is wrong. You have a single allocation `ptr->itemName = malloc (size);` which is then freed, `free(p_sample->itemName);` before the pointer `p_sample->itemName` is lost. (while memory is freed on program exit -- good job for creating good habits and freeing the memory you have allocated!) It is far easier to develop good habits than to break bad ones... – David C. Rankin Jul 30 '20 at 04:51
  • You should not `exit(-1)` returning a negative value to your shell. C defines two macros `EXIT_SUCCESS` (`0`) and `EXIT_FAILURE` (`1`) that should be used instead. The values `0` and `1` are the only two values C defines for returning to indicate success/failure. Further POSIX specifies that only `0` or positive values in the range of `unsigned char` be returned to the shell with those greater than `127` generally reserved for specific purposes. So return `0` or `1` to the shell. – David C. Rankin Jul 30 '20 at 04:56

1 Answers1

0

In addition to the information contained in the comments above -- your biggest problem is you are allocating one-too-few bytes of memory. When you are allocating storage for a string, you must allocate length + 1 bytes to provide room for the nul-terminating character. With you size above1 you are only allocating enough storage for each of the characters, but not room for the nul-terminating character. You must allocate size + 1 bytes.

Additionally, you must validate every user input by checking the return. If you fail to check the return you are inviting Undefined Behavior if the user accidentally enters an incorrect character instead of a digit for any of your numeric input. To validate each input when using scanf() you must check that the return equals the number of conversions specified, e.g.

void readItem (Item *ptr)
{
    int size;
    
    printf ("\nSpecify the amount of letters of the product name: ");
    if (scanf("%d", &size) != 1) {              /* validate every user input */
        fputs ("error: invalid integer input.\n", stderr);
        exit (EXIT_FAILURE);
    }
    ...

That applies to all inputs. You should also consider changing the return type of readItem from void to something that can indicate success or failure of all input. (a simple int and return of 1 or 0 works). That way instead of calling exit on an input or matching failure, you can choose to handle the error and recover instead of exiting.

Additionally, after taking input with scanf() you should empty any characters that remain in stdin until a '\n' or EOF is encountered. This is one of the primary pitfalls with using scanf() for user input1. This is also why it is recommended user input be read with a line-oriented function such as fgets() or getline(). To handle this situation when using scanf() a simple helper function can be called after each input to empty stdin, e.g.

/* function to input stdin to end of line or EOF */
void empty_stdin (void)
{
    int c = getchar();
    
    while (c != '\n' && c != EOF)
        c = getchar();
}

Now you can ensure there are no extraneous characters left in stdin that will cause your next input to fail. Completing the input of size, you would do:

    printf ("\nSpecify the amount of letters of the product name: ");
    if (scanf("%d", &size) != 1) {              /* validate every user input */
        fputs ("error: invalid integer input.\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */
    ...

(note: normally you do not ask the user to enter the number of characters for the next input -- instead you use a sufficiently sized temporary buffer to store the input and call strlen() on the buffer to get the number of characters)

With size containing the number of characters (in your case -- the length of the string), you must allocate size + 1 bytes to provide room to store the nul-terminating character ('\0' -- or just plain 0). You would need:

    ...
    ptr->itemName = malloc (size + 1);  /* you must allocate +1 chars for '\0' */

    if (ptr->itemName == NULL) {        /* validate every allocation */
        perror ("malloc-ptr->itemName");
        exit (EXIT_FAILURE);
    }
    ...

(note: on failure malloc sets errno allowing you to output the error with perror())

Completing your readItem() function, you would do:

    ...
    printf ("\nSpecify the name of the product (MAX %d letters): ", size);
    if (scanf ("%s", ptr->itemName) != 1) {
        fputs ("error: read error ptr->itemName\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */

    printf ("\nHow many products do the company have? ");
    if (scanf ("%d", &ptr->quantity) != 1) {
        fputs ("error: invalid integer input.\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */

    printf ("\nHow expensive is the product? ");
    if (scanf ("%f", &(ptr->price)) != 1) {
        fputs ("error: invalid float input.\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */

    ptr->amount = ptr->price * ptr->quantity;
}

Another issue you may want to address is your placement of '*' in the declaration of your pointer values. Generally the '*' goes with the variable, not the type. Why?

int* a, b, c;

does NOT declare three pointers to int, instead it declares one integer pointer and two integers. Placing the '*' with the variable makes that clear, e.g.

int *a, b, c;

Now putting it altogether and writing a short printItem() function, you could do:

#include <stdio.h>
#include <stdlib.h>

typedef struct Item {
    char *itemName;
    int quantity;
    float price;
    float amount;
} Item;

/* function to input stdin to end of line or EOF */
void empty_stdin (void)
{
    int c = getchar();
    
    while (c != '\n' && c != EOF)
        c = getchar();
}

void readItem (Item *ptr)
{
    int size;
    
    printf ("\nSpecify the amount of letters of the product name: ");
    if (scanf("%d", &size) != 1) {              /* validate every user input */
        fputs ("error: invalid integer input.\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */

    ptr->itemName = malloc (size + 1);  /* you must allocate +1 chars for '\0' */

    if (ptr->itemName == NULL) {        /* validate every allocation */
        perror ("malloc-ptr->itemName");
        exit (EXIT_FAILURE);
    }
    
    printf ("\nSpecify the name of the product (MAX %d letters): ", size);
    if (scanf ("%s", ptr->itemName) != 1) {
        fputs ("error: read error ptr->itemName\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */

    printf ("\nHow many products do the company have? ");
    if (scanf ("%d", &ptr->quantity) != 1) {
        fputs ("error: invalid integer input.\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */

    printf ("\nHow expensive is the product? ");
    if (scanf ("%f", &(ptr->price)) != 1) {
        fputs ("error: invalid float input.\n", stderr);
        exit (EXIT_FAILURE);
    }
    empty_stdin();              /* empty all extraneous characters from stdin */

    ptr->amount = ptr->price * ptr->quantity;
}

void printItem (Item *pitem)
{
    printf ("\nitemName   : %s\n"
            "  quantity : %d\n"
            "  price    : %.2f\n"
            "  amount   : %.2f\n",
            pitem->itemName, pitem->quantity, pitem->price, pitem->amount);
}

int main (void) {
    
    Item sample;
    Item *p_sample = &sample;

    readItem(p_sample);
    printItem(p_sample);

    free(p_sample->itemName);
}

Example Use/Output

$ ./bin/readitem

Specify the amount of letters of the product name: 9

Specify the name of the product (MAX 9 letters): lollypops

How many products do the company have? 100

How expensive is the product? .79

itemName   : lollypops
  quantity : 100
  price    : 0.79
  amount   : 79.00

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to ensure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/readitem
==9619== Memcheck, a memory error detector
==9619== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==9619== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==9619== Command: ./bin/readitem
==9619==

Specify the amount of letters of the product name: 9

Specify the name of the product (MAX 9 letters): lollypops

How many products do the company have? 100

How expensive is the product? .79

itemName   : lollypops
  quantity : 100
  price    : 0.79
  amount   : 79.00
==9619==
==9619== HEAP SUMMARY:
==9619==     in use at exit: 0 bytes in 0 blocks
==9619==   total heap usage: 3 allocs, 3 frees, 2,058 bytes allocated
==9619==
==9619== All heap blocks were freed -- no leaks are possible
==9619==
==9619== For counts of detected and suppressed errors, rerun with: -v
==9619== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Using fgets() For User-Input

As noted earlier, there are a number of pitfalls surrounding the user of scanf() for user input that inherently results in a fragile input routine at best unless all potential pitfalls are addressed. Using a temporary buffer (sufficiently sized character array) to store user input using fgets() and then parsing any values needed with sscanf() ensures that a complete line of input is read each time and no extraneous characters remain to cause the next input to fail. It also opens up a number of different methods for parsing the information you want from the temporary buffer in addition to sscanf().

The only caveat to using a line-oriented input function is that the functions read and include the '\n' (generated by the user pressing Enter) in the buffer they fill. So you simply trim the '\n' from the end of the buffer by overwriting it with '\0' when you obtain the length of the input. The most reliable method is using strcspn().

Changing the return type for readItem() to int to allow for an indication of successs or failure to be returned to main(), you could do something similar to the following:

...
#include <string.h>

#define MAXC 1024       /* if you need a constant, #define one (or more) */
...

Set the size of the temporary buffer by declaring a constant large enough to hold any anticipated user input (including the cat stepping on the keyboard). Don't skimp on buffer size. (reduce accordingly if programming for a microcontroller with limited memory)

The function will return EOF if a user generates a manual EOF by pressing Ctrl+d on Linux or Ctrl+z on windows. The functions returns 0 on success or 1 if any error is encountered allowing you to handle the return in main() (you can adjust whether you use 0 for success or failure to suit your needs)

Reading, allocating and copying the input to ptr->itemName becomes:

int readItem (Item *ptr)
{
    char buf[MAXC];     /* buffer to hold each user input */
    size_t len = 0;     /* length of product name */
    
    fputs ("\nProduct name: ", stdout);             /* fputs all that is needed, no conversions */
    if (fgets (buf, MAXC, stdin) == NULL) {         /* read input into buffer and validate */
        fputs ("(user canceled input)\n", stdout);  /* handle error */
        return EOF;                                 /* return EOF if manual EOF generated by user */
    }
    
    len = strcspn (buf, "\n");                      /* get length of chars (includes '\n') */
    buf[len] = 0;                                   /* overwrite '\n' with '\0' */
    
    ptr->itemName = malloc (len + 1);               /* allocate length + 1 bytes */
    if (!ptr->itemName) {                           /* validate allocation */
        perror ("malloc-ptr->itemName");            /* output error (malloc sets errno) */
        return 1;                                   /* return failure */
    }
    
    memcpy (ptr->itemName, buf, len + 1);           /* copy buf to ptr->itemName */

(note: there is no need to use strcpy() to copy to ptr->itemName and have strcpy() scan for the end-of-string all over again since that was already done in the call to strcspn(). Knowing how many bytes you need to copy, you can simply copy that number of bytes with memcpy())

The read an conversion to int and float are basically identical other than the verbiage and conversion specifier used with sscanf(), e.g.

    fputs ("Quantity    : ", stdout);               /* prompt and read quantity */
    if (!fgets (buf, MAXC, stdin)) {
        fputs ("(user canceled input)\n", stdout);
        return EOF;
    }
    if (sscanf (buf, "%d", &ptr->quantity) != 1) {  /* convert to int with sscanf */
        fputs ("error: invalid integer input.\n", stderr);
        return 1;
    }
    
    fputs ("Price       : ", stdout);               /* prompt and read price */
    if (!fgets (buf, MAXC, stdin)) {
        fputs ("(user canceled input)\n", stdout);
        return EOF;
    }
    if (sscanf (buf, "%f", &ptr->price) != 1) {     /* convert to float with sscanf */
        fputs ("error: invalid integer input.\n", stderr);
        return 1;
    }

All that remains is:

    ptr->amount = ptr->price * ptr->quantity;
    
    return 0;
}

Putting an equivalent example together using the updated readItem(), you would have:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define MAXC 1024       /* if you need a constant, #define one (or more) */

typedef struct Item {
    char *itemName;
    int quantity;
    float price;
    float amount;
} Item;

int readItem (Item *ptr)
{
    char buf[MAXC];     /* buffer to hold each user input */
    size_t len = 0;     /* length of product name */
    
    fputs ("\nProduct name: ", stdout);             /* fputs all that is needed, no conversions */
    if (fgets (buf, MAXC, stdin) == NULL) {         /* read input into buffer and validate */
        fputs ("(user canceled input)\n", stdout);  /* handle error */
        return EOF;                                 /* return EOF if manual EOF generated by user */
    }
    
    len = strcspn (buf, "\n");                      /* get length of chars (includes '\n') */
    buf[len] = 0;                                   /* overwrite '\n' with '\0' */
    
    ptr->itemName = malloc (len + 1);               /* allocate length + 1 bytes */
    if (!ptr->itemName) {                           /* validate allocation */
        perror ("malloc-ptr->itemName");            /* output error (malloc sets errno) */
        return 1;                                   /* return failure */
    }
    
    memcpy (ptr->itemName, buf, len + 1);           /* copy buf to ptr->itemName */
    
    fputs ("Quantity    : ", stdout);               /* prompt and read quantity */
    if (!fgets (buf, MAXC, stdin)) {
        fputs ("(user canceled input)\n", stdout);
        return EOF;
    }
    if (sscanf (buf, "%d", &ptr->quantity) != 1) {  /* convert to int with sscanf */
        fputs ("error: invalid integer input.\n", stderr);
        return 1;
    }
    
    fputs ("Price       : ", stdout);               /* prompt and read price */
    if (!fgets (buf, MAXC, stdin)) {
        fputs ("(user canceled input)\n", stdout);
        return EOF;
    }
    if (sscanf (buf, "%f", &ptr->price) != 1) {     /* convert to float with sscanf */
        fputs ("error: invalid integer input.\n", stderr);
        return 1;
    }
    
    ptr->amount = ptr->price * ptr->quantity;
    
    return 0;
}

void printItem (Item *pitem)
{
    printf ("\nitemName   : %s\n"
            "  quantity : %d\n"
            "  price    : %.2f\n"
            "  amount   : %.2f\n",
            pitem->itemName, pitem->quantity, pitem->price, pitem->amount);
}

int main (void) {
    
    Item sample = { .itemName = "" };

    if (readItem (&sample) != 0)
        exit (EXIT_FAILURE);
    
    printItem (&sample);

    free(sample.itemName);
}

Example Use/Output

$ ./bin/readitem_fgets

Product name: lollypops
Quantity    : 100
Price       : .79

itemName   : lollypops
  quantity : 100
  price    : 0.79
  amount   : 79.00

It equally handles whitespace separated words as well and strings of any length up to MAXC - 1 characters:

$ ./bin/readitem_fgets

Product name: "My dog has fleas and my cat has none?" (McGraw-BigHill 1938)
Quantity    : 10
Price       : 19.99

itemName   : "My dog has fleas and my cat has none?" (McGraw-BigHill 1938)
  quantity : 10
  price    : 19.99
  amount   : 199.90

Memory Use/Error Check

Same output as above with the same number of bytes allocated and results as shown with the first example.

That, along with the comments I provided, are the basic nuts-and-bolts approaches to user input, the different considerations, and the way you handle allocating for string input. Look things over and let me know if you have further questions.

Footnotes:

1. scanf() is full of pitfalls for new C programmers. The recommended way to take user input is with a line-oriented function like fgets() or POSIX getline(). That way you can ensure a complete line of input is read and that characters do not remain in your input buffer (stdin) in the event of a matching failure. A few links discussing the proper use of scanf, C For loop skips first iteration and bogus number from loop scanf and Trying to scanf hex/dec/oct values to check if they're equal to user input and How do I limit the input of scanf to integers and floats(numbers in general)

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
  • This answers helped me so much. Do you know any good memory usage checker for Windows? And I´m already studying this treatments of errors and recommended ways of user-input. Always improving! – Marcellus T. Biancardi Jul 30 '20 at 14:14
  • There is a collection of good approaches for windows here [Is there a good Valgrind substitute for Windows?](https://stackoverflow.com/questions/413477/is-there-a-good-valgrind-substitute-for-windows). Last benefit I forgot to mention with the `fgets()/sscanf()` approach is you get to independently validate (1) the read of information from the user with `fgets()` and (2) the validation of the conversion with `sscanf()`. This allows you to write robust input routines that require to user to enter the correct value by looping continually until that input is provided. Good luck with your coding! – David C. Rankin Jul 30 '20 at 18:30