1

I tried 2 different approaches to build a function that accepts string input from user and stores it in a variable.

Implementation A

char* input(size_t size)
{
    char buffer[size];

    char* str = (char*)malloc(sizeof(char) * sizeof(buffer));

    if (fgets(buffer, size, stdin) != NULL)
    {
        buffer[strcspn(buffer, "\n")] = '\0';

        printf("Buffer says: %s\n", buffer);

        strncpy(str, buffer, sizeof(buffer));

        str[sizeof(buffer) - 1] = '\0';
        
        printf("str inside function says: %s\n", str);

        return str;
    }  

    return NULL;
}

Implementation B

int input2(char* str, size_t size)
{
    char buffer[size];
    
    str = (char*)malloc(sizeof(char) * sizeof(buffer));

    if (fgets(buffer, size, stdin) != NULL)
    {
        buffer[strcspn(buffer, "\n")] = '\0';

        printf("Buffer says: %s\n", buffer);

        strncpy(str, buffer, sizeof(buffer));

        str[sizeof(buffer) - 1] = '\0';
        
        printf("str inside function says: %s\n", str);

        return 0;
    }  

    return -1;
}

main.c:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "libs/custom/myio.h"

int main(int argc, char const *argv[])
{
    char *ip1;
    
    ip1 = input(1000);
    printf("ip1 inside main says %s\n", ip1);
    free(ip1);

    char* ip2;

    input2(ip2, 1000);
    printf("ip2 inside main says %s\n", ip2);
    free(p2);

    return 0;
}

When running the program:

Hi                             # user input
Buffer says: Hi
str inside function says: Hi
ip1 inside main says Hi
Hi                             # user input
Buffer says: Hi
str inside function says: Hi
ip2 inside main says (null)

A works perfectly but B doesn't. A is not the approach that I want to use, I prefer to use B instead.

When I tried debugging with valgrind along with gdb, it seems that it is detecting errors in the printf function (??).

==177193== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
==177193== 
==177193== 1 errors in context 1 of 2:
==177193== Conditional jump or move depends on uninitialised value(s)
==177193==    at 0x48D71C2: __vfprintf_internal (vfprintf-internal.c:1688)
==177193==    by 0x48C1EBE: printf (printf.c:33)
==177193==    by 0x109276: main (main.c:19)
==177193==  Uninitialised value was created by a stack allocation
==177193==    at 0x109209: main (main.c:7)
==177193== 
==177193== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

I have done a lot of research but I am still getting no where on why B is not working.

John Kugelman
  • 349,597
  • 67
  • 533
  • 578
Zaki
  • 107
  • 9
  • 2
    1) Your two functions are identical and both allocate a buffer with `malloc`, you will need to `free()` that in any case. 2) Your second function should take `char **` if you want to allocate a buffer for the caller (and then do `*str = malloc(...)`, otherwise you're just overwriting a local variable and then losing reference to the allocated buffer when the function return. 3) What is all the fuss with having an additional completely unneeded buffer on the stack? Just do all the operations on the allocated one. – Marco Bonelli Jan 23 '22 at 01:54
  • @MarcoBonelli That's correct I edited the question that was my bad, I have been trying to do **B** without malloc but I was getting seg fault so I used `malloc` when things didn't seem to be working but failed too. – Zaki Jan 23 '22 at 01:59
  • 1
    You don't need to cast the return value of `malloc`. The whole point of `malloc` returning `void *` is so that you don't have to cast. – Shambhav Jan 23 '22 at 02:27

1 Answers1

1

The problem in the function input2 ("Implementation B") is that the line

str = (char*)malloc(sizeof(char) * sizeof(buffer));

will only modify the pointer variable str in the function input2, which contains a copy of the value of the variable ip2 in the function main. It will not set the value of the original variable ip2 in the function main.

If you want the variable ip2 to be affected by this line of code, then, instead of passing the value of the variable ip2 to the function input2, you should instead pass a pointer to this variable, by changing the parameters of the function to the following:

int input2( char** str, size_t size )

and by changing the line

str = (char*)malloc(sizeof(char) * sizeof(buffer));

to:

*str = (char*)malloc(sizeof(char) * sizeof(buffer));

Now, when calling the function input2 in main, instead of passing the value of ip2, you should instead pass the address of that variable, like this:

input2( &ip2, 1000 );

Now, this variable should also be affected by the changes in the function input2.

Also, it is worth noting that in C (in contrast to C++), it is not necessary to cast the result of malloc. See this question for further information: Do I cast the result of malloc?

Andreas Wenzel
  • 22,760
  • 4
  • 24
  • 39
  • I tried what you said, I also ditched the buffer because I didn't notice that is useless, so it works now but the mechanism that I have for removing the new line `*str[strcspn(*str, "\n")] = '\0';` fails now. How can I go about that? – Zaki Jan 23 '22 at 02:12
  • Also what If I don't want to `malloc` in this function since I would force the user to free memory everytime they call this function, is it possible to do it without `malloc`? – Zaki Jan 23 '22 at 02:13
  • 1
    @Zaki: If you don't want the callee function (`input2`) to supply the memory buffer, then the caller function (`main`) will have to supply the memory buffer. This buffer does not necessarily have to be dynamically allocated, it can also be a fixed-length array. That way, you won't have to use `malloc` or `free`. You could make the function `input2` take two parameters: 1. A pointer to the start of the array, and 2. the size of the array. That is all the information that the function `input2` needs to use the array. – Andreas Wenzel Jan 23 '22 at 02:20
  • 1
    @Zaki: According to the [rules on operator precedence](https://en.cppreference.com/w/c/language/operator_precedence), writing `*str[strcspn(*str, "\n")] = '\0';` is equivalent to writing `*(str[strcspn(*str, "\n")]) = '\0';`. This is not what you want. You should write `(*str)[strcspn(*str, "\n")] = '\0';` instead. – Andreas Wenzel Jan 23 '22 at 02:25