C: using sprintf and strncpy inserting data into an array of pointers

Question

Hello,

I have a structure that has an array of pointers. I would like to insert into the array digits in string format, i.e. "1", "2", etc..

However, is there any difference in using either sprintf or strncpy?

Any big mistakes with my code? I know I have to call free, I will do that in another part of my code.

Many thanks for any advice!

struct port_t
{
    char *collect_digits[100];

}ports[20];

/** store all the string digits in the array for the port number specified */
static void g_store_digit(char *digit, unsigned int port)
{
    static int marker = 0;
    /* allocate memory */
    ports[port].collect_digits[marker] = (char*) malloc(sizeof(digit)); /* sizeof includes 0 terminator */
    // sprintf(ports[port].collect_digits[marker++], "%s", digit);
    strncpy(ports[port].collect_digits[marker++], digit, sizeof(ports[port].collect_digits[marker]));
}

Answer 1

The first thing: why have an array of pointers? Do you expect multiple strings for a port object? You probably only need a plain array or a pointer (since you are malloc-ing later on).

struct port_t
{
    char *collect_digits;
}ports[20];

You need to pass the address of the string, otherwise, the malloc acts on a local copy and you never get back what you paid for.

static void g_store_digit(char **digit, unsigned int port);

Finally, the sizeof applies in a pointer context and doesn't give you the correct size.

Answer 2

Yes, your code has a few issues.

• In C, don't cast the return value of malloc(). It's not needed, and can hide errors.
• You're allocating space based on the size of a pointer, not the size of what you want to store.
• The same for the copying.
• It is unclear what the static marker does, and if the logic around it really is correct. Is port the slot that is going to be changed, or is it controlled by a static variable?

Do you want to store only single digits per slot in the array, or multiple-digit numbers?

Here's how that function could look, given the declaration:

/* Initialize the given port position to hold the given number, as a decimal string. */
static void g_store_digit(struct port_t *ports, unsigned int port, unsigned int number)
{
  char tmp[32];

  snprintf(tmp, sizeof tmp, "%u", number);
  ports[port].collect_digits = strdup(tmp);
}

Answer 3

Instead of using malloc() and strncpy(), just use strdup() - it allocates the buffer bin enough to hold the content and copies the content to the new string, all in one shot.

So you don't need g_store_digit() at all - just use strdup(), and maintain marker on the caller's level.

Answer 4

strncpy(ports[port].collect_digits[marker++], digit, sizeof(ports[port].collect_digits[marker]));

This is incorrect.

You have allocated onto collect_digits a certain amount of memory.

You copy char *digits into that memory.

The length you should copy is strlen(digits). What you're actually copying is sizeof(ports[port].collect_digits[marker]), which will give you the length of a single char *.

You cannot use sizeof() to find the length of allocated memory. Furthermore, unless you know a priori that digits is the same length as the memory you've allocated, even if sizeof() did tell you the length of allocated memory, you would be copying the wrong number of bytes (too many; you only need to copy the length of digits).

Also, even if the two lengths are always the same, obtaining the length is this way is not expressive; it misleads the reader.

Note also that strncpy() will pad with trailing NULLs if the specified copy length is greater than the length of the source string. As such, if digits is the length of the memory allocated, you will have a non-terminated string.

The sprintf() line is functionally correct, but for what you're doing, strcpy() (as opposed to strncpy()) is, from what I can see and know of the code, the correct choice.

I have to say, I don't know what you're trying to do, but the code feels very awkward.

Answer 5

Another problem with the original code: The statement

strncpy(ports[port].collect_digits[marker++], digit, sizeof(ports[port].collect_digits[marker]));

references marker and marker++ in the same expression. The order of evaluation for the ++ is undefined in this case -- the second reference to marker may be evaluated either before or after the increment is performed.