Refactoring C Argument Variables

Question

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

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

It works fine, 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 use 2 "temporary" buffers here, because first I need to copy the original message from the function arguments to a temporary buffer to add "\ r \ n" to it, and then copy this temporary buffer to another temporary buffer for actual formatting with the arguments provided from the function call, and only THEN can I send stuff in my path.

How can I make it cleaner, better?

---

Thanks for all the input here, I thought my only problem was the mess there, but it was actually a ticking bomb! My new feature 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 would accept a few answers here, but meh.

Answer 1

First use vsnprintf to format the data, and then add "\ r \ n" to the result from this. Alternatively, simply use the second send call to send "\ r \ n".

Answer 2

First, if you are interested in a “cleaner”, stop using strncpy . Despite the misleading name (and contrary to popular belief), this is not a function for copying strings with a limited length. It is safe to say that strncpy is a function that hardly uses today. When you see strncpy that is used in the code, the “cleaner” immediately becomes unstable (except that it was actually intended to be used by the one that was originally a limited set of strncpy cases).

Actually, your code is violated intentionally because you used strncpy : if the format string is longer than the buffer, strncpy does not add a terminating null character to the result, which means that a subsequent call to strncat will fail (at best).

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

Your code is also corrupted due to misuse of strncat : strncat does not accept the length of the full buffer as the last argument. Instead, strncat expects the length of the available remaining buffer. So, if you want to use strncat , you first need to calculate how much free space is left at the end of the buffer after the previous copy. Again, even if strncat more useful than strncpy , you might be better off using the non-standard (but often provided by the implementation) function of strlcat , which actually works as you thought strncat .

Secondly, instead of adding the \r\n , why don't you do it later? Use the fact that vsnprintf returns the number of characters written to the output buffer, and simply appends the characters \r , \n and \0 at the end after vsnprintf completes. For this purpose you do not need to use strncat . Just write the characters to the buffer directly, making sure, of course, that you are not crossing the border.

Answer 3

Since \r\n will end at the end of the formatted string, why not copy later:

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

Note that I also fixed the strncat arguments.

Answer 4

If you do not want to use msg for strcat (it is unsafe and malicious, because you do not know the size of the line), I think you will have to live with two buffers.

As an aside, I would consider strncpy (..., BUFSIZE-2) so that \ r \ n always does this on your posts, and therefore strings are always wrapped.

Answer 5

One of the main problems with your code is that vsnprintf returns the number of characters to be buffered if it was infinitely large, which could be more than BUFSIZE if the buffer is not large enough. Therefore, if you have a message that overflows, you end up sending random garbage after the end of your buffer. You need to add a line

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

after vprintf if you want to actually trim the message