C-string appender for buffers and strings

Question

Is this C appender safe, and reasonably optimal?

#define REALLOC_BUFFER_SIZE 4096
char *append(char *orig_str, size_t *orig_str_size, const char *append_str) {
 // no action required if appending an empty string
 if (append_str[0] == 0) {
 return orig_str;
 }
 // does the current char* have enough space?
 size_t req_space = strlen(orig_str) + strlen(append_str) + 1; // +1 for NULL byte
 // resize if the required space is greater than the original buffer size.
 if (req_space > *orig_str_size) {
 // prepare the variable with the amount of space we want
 size_t allocate = *orig_str_size + req_space + REALLOC_BUFFER_SIZE;
 char *new_str = (char *)realloc(orig_str, allocate);
 // resize success.. 
 if (new_str == NULL) {
 fprintf(stderr, "Couldn't reallocate memory\n");
 return NULL;
 }
 // the realloc didn't fail, here is our new pointer
 orig_str = new_str;
 // overwrite the free space with NULL's
 memset(orig_str + strlen(orig_str), 0, req_space);
 // copy the appending text to the string
 strcpy(orig_str + strlen(orig_str), append_str);
 // modify the original size
 *orig_str_size = allocate;
 } else {
 // append the string
 strcpy(orig_str + strlen(orig_str), append_str);
 }
 return orig_str;
}

Example usage:

char *buffer;
size_t buffer_size = 20;
int i;
buffer = (char *)malloc(sizeof(char) * buffer_size);
strcpy(buffer, "1234567890123456789"); // fill some data prior.. 
// original string
for (i=1; i < 100; ++i) {
 buffer = append(buffer, &buffer_size, "AABBCC");
}
free(x);

Latest changes: GitHub

• SirPython: In your title, please only state the function/purpose of your code; the body of your post is where the questions should go.

Answer 1

In addition to what the other answers say and I generally agree with:

Don't call strlen more than once. That's just a waste. Count the length of orig_str and append_str once at the beginning of the function and store them away in const variables. That will also make your initial check whether append_str is empty look cleaner:

const size_t append_len = strlen(append_str);
if (!append_len)
 return orig_str;
const size_t orig_len = strlen(orig_str);

Moving the strlen(orig_str) above the conditional would look cleaner in my eyes but it might be a pessimization if append_str is often empty.

Of course, the most efficient way would be if you could pass orig_len as an additional input parameter. Or you could use a

struct string_buffer
{
 size_t length;
 size_t capacity;
 char * data;
};

that encapsulates all the required info and change your function's signature to

ssize_t
append_string(struct string_buffer * buffer, const char * appendix);

where the function could return the number of bytes successfully appended or –1 on error.

My second remark would be that you are growing your buffer by a constant amount each time you reallocate. It should really grow exponentially unless you really know that linear growth will be preferable in your specific use-case, which I find hard to imagine how that could be.

Concerning your comments: there is no NULL byte. NULL is a macro constant for the null pointer. The null byte is abbreviated as NUL in the ASCII character set. I would also find it helpful to have your function's semantics exactly documented in a comment. For example, should the user free the buffer if it fails and returns NULL? Will the existing data be modified in that case? I can try answering those questions by reading your implementation but that doesn't allow me to distinguish a bug in your code from a different valid design decision.

Answer 2

• strcat(orig_str, append_str) looks cleaner than a strcpy/strlen combo.

• memset is pointless. You need only 1 terminating zero, and it is taken care of by string routines.

• The responsibilities of if (req_space > *orig_str_size) and a corresponding else clauses overlap. The only task of the if clause should be string reallocation. Copying needs to be performed in any case. Consider instead

  if (req_space > *orig_str_size) {
   allocate = ...;
   char *new_str = realloc(orig_str, allocate);
   if (new_str == NULL) {
   handle_error;
   }
   *orig_str_size = allocate;
   orig_str = new_str;
  }
  strcat(orig_str, append_str);
  

• fprintf doesn't belong to a such low level routine. Returning an error condition is enough. It is up to the caller to print or not to print.

Answer 3

This line:

char *new_str = (char *)realloc(orig_str, allocate);

has a problem:

1. In C, do not cast the returned value from realloc (and family).

---

In this line, from the usage example:

buffer = (char *)malloc(sizeof(char) * buffer_size); 

there are a couple of problems:

1. In C, do not cast the returned value from malloc (and family).

2. Always check the returned value (!= NULL) to assure the operation was successful.

Answer 4

Another thing about this function that bugs me. If a realloc happens, the orig_str is invalidated. That goes against my expectation of parameters passed to functions. Instead of returning the new char *, how about changing the pointer to char ** pOrig_str so it can be modified in-place?