Array entries being overwriten in C

Question

I want to convert an integer array to a string array. For example, if arr[] = {1, 2, 3, 4, 5}, I want to end up with arr2[] = {"1", "2", "3", "4", "5"}. This function works fine until it exits the for loop where all the array entries are being overwritten with the value of the last entry. Any idea as to why this might be happening?

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

#define SIZE 5
int main(){
    int nums[] = {1, 2, 3, 4, 5};
    char **strings = (char **)malloc(SIZE * sizeof(char*));

    for(int i = 0; i < SIZE; i++){
        char temp[20];
        sprintf(temp, "%d", nums[i]);
        strings[i] = temp;
        //prints correct values here
        printf("%s\n", strings[i]);
    }

    for(int i = 0; i < SIZE; i++){
        //prints wrong values here
        printf("%s\n", strings[i]);
    }

    free(strings);
    return 0;
}

`temp` is a local variable inside the loop. Each iteration it goes out of scope and its life-time will end. All your pointers will be to this array, so after the loop all your pointers will be invalid. Using it in any way will lead to *undefined behavior*. — Some programmer dude, Nov 08 '19 at 17:42
Not to mention that you iterate five times over an array containing only four elements. — Some programmer dude, Nov 08 '19 at 17:44
@Someprogrammerdude yeah that was a mistake on my part the array is supposed to be {1, 2, 3, 4, 5} — ASlackjaw, Nov 09 '19 at 14:31
Then please edit your question to fix it. A [mcve] with unrelated errors and problems will distract from the actual problem you ask about. — Some programmer dude, Nov 09 '19 at 14:33
Thanks to both of you for bringing my attention to that. It had actually completely slipped my mind, and I honestly do not know why I thought that would work — ASlackjaw, Nov 09 '19 at 14:37

ggorlen · Accepted Answer · 2019-11-08T19:43:02.253

The issue is strings[i] = temp;. This assigns temp to strings[i], but then temp is a local variable scoped to the loop block, and is not valid after the block terminates.

You'll need to malloc memory for each string to hold the copied value (and free it when done). tmp is also unnecessary since we can sprintf directly into the pointer.

SIZE = 5 but your array only has 4 members, so we have an out of bounds access. I'd prefer to scope this to the data it represents rather than make it a global constant. I'm also assuming this array will handle arbitrary data, because as-is, there's no advantage to it over using i + 1 inside your loop.

malloc(12) is sufficient space to hold a 32-bit int string (sizeof char is always 1 and we need space for '-' and '\0' characters). As pointed out in this comment, you can use sizeof(int) * CHAR_BIT / 3 + 2 to compute the correct size for the buffer where CHAR_BIT is defined in the limits.h header.

As an aside, there's no need to cast malloc and it's good practice to use sizeof(*strings) in case the pointer type changes during a refactor.

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

int main() {
    int nums[] = {1, 2, 3, 4};
    int nums_size = 4;
    char **strings = malloc(nums_size * sizeof(*strings));

    for (int i = 0; i < nums_size; i++) {
        strings[i] = malloc(12);
        sprintf(strings[i], "%d", nums[i]);
    }

    for (int i = 0; i < nums_size; i++) { 
        printf("%s\n", strings[i]);
        free(strings[i]);
    }

    free(strings);
    return 0;
}

Thanks for the help and advice, your solution worked best for what I'm trying to do! — ASlackjaw, Nov 09 '19 at 14:50

Steve Friedl · Answer 2 · 2019-11-08T19:41:42.900

2

Always let the compiler do counting for you.

First, I define below a macro COUNTOF that produces the number of items (not the number of bytes) in the array object; that should be used everywhere.

Second, the strdup() library function makes a copy of a string buffer - it counts the length, allocates the right number of bytes, then copies the string into it. Much easier than rolling your own.

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

#define COUNTOF(x)   ( sizeof(x) / sizeof((x)[0]) )

int main(){
    int nums[] = {1, 2, 3, 4};
    char **strings = malloc(COUNTOF(nums) * sizeof(*strings));

    for(int i = 0; i < COUNTOF(nums); i++){
        char temp[40];  // bigger than any plausible int
        sprintf(temp, "%d", nums[i]);
        strings[i] = strdup(temp);   // HERE
        printf("%s\n", strings[i]);
    }

    for(int i = 0; i < COUNTOF(nums); i++){
        //now prints correct value here :-)
        printf("%s\n", strings[i]);
    }

    return 0;
}

edited Nov 08 '19 at 19:41

answered Nov 08 '19 at 18:00

Steve Friedl

3,929
1
23
30

And @ggorlen correctly reminds you to free memory when you're done with it. – Steve Friedl Nov 08 '19 at 18:05
1

Aside: `char temp[20];` is OK for 32 bit math. Another is `char temp[sizeof(int)*CHAR_BIT/3 +2];` or some other similar self-scaling buffer size. – chux - Reinstate Monica Nov 08 '19 at 19:39
Good point; it's 21 bytes to support 64-bit stuff, so there's no harm in just making it 40. – Steve Friedl Nov 08 '19 at 19:41
When I'm looking for quick coding and do not want to compute the buffer need, I think of a likely worst case to support, like int128, and then double it. – chux - Reinstate Monica Nov 08 '19 at 19:43
Please note that `strdup` isn't standard C. – Lundin Nov 08 '19 at 19:54

score 0 · Answer 3 · answered Nov 08 '19 at 20:07

There's actually no need to do this in run-time. You can do this at compile time if you can suffer some "X macro" tricks.

#define INIT_LIST \
   X(1) \
   X(2) \
   X(3) \
   X(4) \

#define STR(n) #n

#include <stdio.h>

int main (void)
{
  #define X(n) n,
    int nums[] = {INIT_LIST};
  #undef X

  #define X(n) STR(n),
    const char* str[] = {INIT_LIST};
  #undef X

  for(size_t i=0; i<sizeof(nums)/sizeof(*nums); i++)
  {
    printf("%d %s\n", nums[i], str[i]);
  }
}

Where INIT_LIST has to contain all the initializers as integers. After pre-processing, the above expands to this:

#include <stdio.h>

int main (void)
{
  int nums[] = {1,2,3,4};
  const char* str[] = {"1", "2", "3", "4"};

  for(size_t i=0; i<sizeof(nums)/sizeof(*nums); i++)
  {
    printf("%d %s\n", nums[i], str[i]);
  }
}

Performance-wise, this is naturally going to be astronomically faster than any solution using heap allocation. The down side is that X macros can be a bit hard to read.

Array entries being overwriten in C

3 Answers3