C variable argument refactoring

Question

I have a function irc_sendline that can be called like printf can

irc_sendline(s, "A strange game.\nThe only %s is not to play.", "winning move");

It works perfectly, but I'm not happy with its implementation:

int irc_sendline(irc *iobj, char *msg, ...)
{
   char tmp_msg[BUFSIZE], fmsg[BUFSIZE];
   va_list args;
   int len;

   va_start(args, msg);

   strncpy(tmp_msg, msg, BUFSIZE);
   strncat(tmp_msg, "\r\n", BUFSIZE);

   len = vsnprintf(fmsg, BUFSIZE, tmp_msg, args);
   len = send(iobj->fd, fmsg, len, 0);

   return len;
}

You see, I'm using 2 "temporary" buffers here, because I first have to copy the original message from the function arguments to a temporary buffer to append "\r\n" to it, and then copy that temporary buffer to another temporary buffer to do the actual formatting with the arguments supplied from the function call, and only THEN I can send the stuff on its way.

How could I make this cleaner, better?

---

Thanks for all the input here, I thought my only problem was the mess in there, but it was actually a ticking timebomb! My new function looks like this:

int irc_sendline(irc *iobj, char *msg, ...)
{
   char buffer[BUFSIZE];
   va_list args;
   int res_str_len;
   int sent;

   va_start(args, msg);

   res_str_len = vsnprintf(buffer, BUFSIZE, msg, args);

   sent =  send(iobj->fd, buffer, res_str_len, 0);
   sent += send(iobj->fd, "\r\n", 2, 0);

   return sent;
}

If I could, I'd accept multiple answers here, but meh.

Answer 1

Since the \r\n is going to end up at the end of the formatted string, why not copy afterwards:

va_start(args, msg);
len = vsnprintf(fmsg, BUFSIZE, msg, args);
strncat(fmsg, "\r\n", BUFSIZE - strlen(fmsg) - 1);

Note that I also fixed the arguments to strncat.

Answer 2

First use vsnprintf to format the data, then append the "\r\n" to the result from that. Alternatively, just use a second call to send to sent the "\r\n".

Answer 3

Unless you wanted to use msg for the strcat (unsafe and evil because you don't know the string size), I think you are going to have to live with 2 buffers.

As an aside, I'd consider strncpy(..., BUFSIZE-2) so that the \r\n always makes it onto your messages and therefore strings always wrap.

Answer 4

Firstly, if you are interested in "cleaner", stop using strncpy. Despite the misleading name (and contrary to the popular belief), this is not a limited-length string copying function. It is safe to say that strncpy is a function that has virtually no practical uses today. When you see strncpy used in the code, "cleaner" immediately becomes out of question (aside from the disappearingly narrow set of the cases strncpy was actually intended to be used for).

In fact, your code is broken specifically because you used strncpy: if the length of the format string is greater than the length of the buffer, strncpy does not append the terminating null character to the result, meaning that the subsequent strncat call will crash (at best).

The limited length copying function does not exist in the standard library, but it is often provided by the implementation under the name strlcpy. If the implementation you are using does not provide one - write one yourself and use it.

Your code is also broken because of the incorrect use of strncat: strncat does not take the length of the full buffer as the last argument. Instead, strncat expects the length of the available remainder of the buffer. So, if you want to use strncat, you have to calculate first how much space was left at the end of the buffer after the previous copying. Again, even though strncat is more useful than strncpy, you might be better off using the non-standard (but often provided by the implementation) function strlcat, which actually works the way you thought strncat worked.

Secondly, instead of adding the \r\n part in advance, why don't you do it afterwards? Use the fact that vsnprintf returns the number of characters written into the output buffer and simply append the \r, the \n and \0 characters at the end after vsnprintf finished working. You don't have to use strncat for that purpose. Just write the characters into the buffer directly, making sure, of course, that you don't cross the boundary.

Answer 5

One major problem with your code -- vsnprintf returns the number of characters that would be placed in the buffer if it was infinitely large, which may be larger than BUFSIZE if the buffer is not big enough. So if you have a message that overflows, you'll end up sending random garbage from after the end of your buffer. You need to add a line

if (res_str_len >= BUFSIZE) res_str_len = BUFSIZE-1

after the vprintf if you want to actually truncate the message