6

Consider the following function:

char* color(const char* clr, char* str)
{
    char *output = malloc(strlen(str)+1);

    /* Colorize string. */
    sprintf(output, "%s%s%s", clr, str, CLR_RESET);

    return output;
}

The above function allow us to print colorized messages in linux terminal.

So I can write

printf("%s", color(CLR_RED, "This is our own colorized string"));

and see the message This is our own colorized string in red.

My concern is the output string allocated in color(). By the time the function returns an allocated element (array in our example) we somewhat need to deallocate it (C does not have a garbage collector).

My question is, what happens to the output after is passed to printf()? When we exit the function printf() the array is still allocated? If it is, how we can deallocate it? Is there any way to write a function that do this for us?

Thanks in advance!

Jens
  • 69,818
  • 15
  • 125
  • 179

3 Answers3

9

First, your code is wrong. You call malloc with a too small size so will have a buffer overflow. And you forgot to test against failure of malloc. You probably should code:

char *output = malloc(strlen(clr)+strlen(str)+strlen(CLR_RESET)+1);
if (!output) { perror ("malloc in color"); exit(EXIT_FAILURE); }
sprintf(output, "%s%s%s", clr, str, CLR_RESET);

BTW, some systems have asprintf (which would be easier to use). IMHO using sprintf is dangerous (you should prefer snprintf).

Then, C programming requires a lot of conventions. You should have yours (inspired by usual practice). I recommend to study existing free software source code (e.g. from github) for inspiration.

You can have a function returning a malloc-ed pointer, but your need to document that convention (at the very least, as a comment in the common header file declaring color) and explicit the obligation to call free on its result, and follow it elsewhere.

Then, in the function calling color you would call free on the result of color.

In general, you should free a pointer value after malloc has been called to obtain it (but of course, take care of pointer aliases), but only once that pointer (more precisely the memory zone it points to) is useless.

Don't forget to enable all warnings & debug info when compiling (so gcc -Wall -g if using GCC). Use the debugger (gdb). Use memory leak detectors like valgrind.

You should read a lot more about C dynamic memory allocation & virtual address space.

If on Linux, read also Advanced Linux Programming. And reading about Operating Systems and about Garbage Collection will be helpful (at least for relevant concepts).

Community
  • 1
  • 1
Basile Starynkevitch
  • 223,805
  • 18
  • 296
  • 547
  • `You call malloc with a too small size` what do you mean by that? –  Feb 26 '17 at 16:50
  • 2
    @GeorgeGkas you allocate say 3 bytes and write 15. I'd advise you to read more on basics of C before proceeding, judging from your comments. – Giorgi Moniava Feb 26 '17 at 16:50
  • 1
    @GeorgeGkas your color codes are probably 5 bytes long each and you need two sequences for around 10 bytes. Your output will be that many bytes longer than you've allocated, every time. Behavior is undefined once you write out of bounds in your sprintf call. – casey Feb 27 '17 at 00:03
  • @GeorgeGkas You need to allocate `strlen(clr) + strlen(str) + strlen(CLR_RESET) + 1` bytes to hold the resulting string. – David Conrad Feb 27 '17 at 14:04
3

My question is, what happens to the output after is passed to printf()? When we exit the function printf() the array is still allocated? If it is, how we can deallocate it? Is there any way to write a function that do this for us?

Yes, after color function ends, the memory you allocated for output is still there. You need to store the return value of color function, and call free when you are done using it.

Yes, and as noted in another answer, the size you pass to malloc is small in this case.

You can avoid using malloc altogether though. Declare a string in the caller code, e.g.

char someStr[100];

and pass to your color function

void color(char* output, const char* clr, char* str)
{
    /* Colorize string. */
    sprintf(output, "%s%s%s", clr, str, CLR_RESET);
}

Here, no need for malloc anymore.

Call: color(someStr, CLR_RED, "This is our own colorized string")

Giorgi Moniava
  • 27,046
  • 9
  • 53
  • 90
0

Another way is to use a global buffer char[whatever]: no need to alloc(), no need to free(). The color() functions always returns that buffer. The only problem, which could be a non-problem, is that color() will be NOT reentrant and must be called just once per statement.

char g_color_buf[200];

char* color(const char* clr, char* str) {
    sprintf(g_color_buf, "%s%s%s", clr, str, CLR_RESET);
    return g_golor_buf;
}

UPDATE: from a comment below, it is even better to declare g_color_buf[] as static inside the function itself, instead of polluting the global namespace. Doing so does not cure the other problems, but is better anyway.

As said above, this routine should not be used in statements like:

printf("%s %s", color(CL_WHITE, "Result:"), color(CL_RED, "Error"));

because the second call to color() would destroy the work done by the first call.

Finally, to reply to your question (needed, because otherwise someone can say "you didn't reply"), the memory allocated by color() in your example code will stay there until you (no one else) will free it. Considering the mean, normal use you presumably want, it would be uncomfortable to write many times:

tmp = color(CL_RED, "hello");
printf("%s", tmp);
free(tmp);

The solution to use a global buffer to pass along to color(), and perhaps accompanied by its size, like:

printf("%s", color(buffer1, CL_RED, "Hello"));
--or--
printf("%s", color(buffer2, sizeof(buffer2), CL_RED, "HelloHello"));

is certainly the most correct, but boring. Well, it is not boring if you like to type!

linuxfan says Reinstate Monica
  • 4,281
  • 2
  • 19
  • 35
  • 1
    That was included in the latest edition of [Georgi's answer](http://stackoverflow.com/a/42471214/841108) before you wrote yours. – Basile Starynkevitch Feb 26 '17 at 17:22
  • @BasileStarynkevitch. No. Read well. I'm talking about a global buffer which is not passed to the function as argument. Quite different, simpler to invoke, and _not reentrant_, differently from Giorgi's answer. – linuxfan says Reinstate Monica Feb 26 '17 at 17:33
  • Why a global? why not a `static` function-local? Also, the problem with reentrancy is not that "you must call it just once per statement": it's that there cannot be any situation where more than one site might call it in a potentially interleaved manner, otherwise UB occurs. It is also not that "the second call to color() would destroy the work done by the first call"; why would that be a problem? The real problem is that the sequencing of the two calls is undefined (as they're function arguments), & so they need not occur sequentially (non-interleaved) at all, never mind in the order written – underscore_d Feb 26 '17 at 23:07
  • 1
    @underscore_d: I updated the answer about static local. Good point. About the double call of color() in printf(): it is a problem because, in the end, printf receives two pointers pointing to the same buffer. Clearly, this buffer can not hold _two different contents_ at the same time, so printf() will print two times the same string, which is not the desired result. – linuxfan says Reinstate Monica Feb 27 '17 at 07:37
  • @EJP. You are right. But neither printf() is guaranteed to be thread safe. The OP talks about C, not Posix, and his example is seems printf()-centric. I can imagine that this color() function could be used in different contexts than printing to the console, though. – linuxfan says Reinstate Monica Feb 27 '17 at 07:44
  • @linuxfan Right, I was wrongly envisioning how the call would occur, somehow imagining that `printf` would evaluate each argument separately (though not in a specific order) during execution, which in retrospect is clearly wrong; all arguments must be evaluated before execution of the function's body begins, but the order of that evaluation is unspecified. – underscore_d Feb 27 '17 at 15:44