Input memory buffer exceeding situation handling

Please review the following code:

static int md5_encode(const char *  input_md5  , char *  output  , int size)
{
  // checking pre-requisits //
  if( output == NULL || input_md5 == NULL || size == 0)
  {
   printf( "main.cpp:md5_encode() error check for the prerequisites. \n");
   exit(0);
  }

  int input_length = strlen(input_md5);

  // null terminate the output first //
  output[0] = 0;
  char * buffer [10];

  for (int i=0; i<input_length,i++)
  {
    char current_char = input_md5 [i];
    // encode it //
    strcat(output,"uc");
    sprintf(buffer,"%d",current_char);
    strcat(output,buffer);

  }
  // check for the buffer overflow condtions //
  // if something goes out of the buffer then assert //
  if(size < strlen(output) )
  {
    printf("Warning: Possible buffer-overflow\n"); 
    return 0;
  }

  // return on success //
  return 1;
}

As you can see, what this does is to encode an md5 string into ucuc format. The output buffer is allocated by the caller and its size and address are passed as parameters.

The function just writes to the output buffer with 'strcat()' without any checking. Then afterwards it checks whether 'size' is less than the amount that was written. If it exceeds the limit, then the function gives an error message.

Any more suggestions than just a warning message? How could I improve this?

I'm not familiar with the "ucuc format", or what such an output format could be useful for. A Google search for md5 ucuc format shows this question itself as the only relevant hit.

What are the expected inputs? A 32-character string of hexadecimal digits? If so, the longest possible output should be 32 repetitions of uc102, corresponding to an input of ffffffffffffffff. (If the input can contain arbitrary characters, then allow for 6 bytes of output per byte of input, since there might be a negative sign.) Why not just document the requirement for the caller to pass you an output buffer capable of storing 161 bytes? Then do away with the size parameter. You could return the number of bytes of output (excluding the \0 terminator).

Printing the error messages is not advised; it contaminates the output, and side effects limit the ability to reuse your code. If you must print diagnostics, print to stderr instead. You can use the __FILE__ and __FUNCTION__ macros for the error message.

A more common way to handle null inputs is to either let it crash, or use assertions. An assertion failure message typically already includes the line number. If you use assertions, use three separate assertions, not a combined one. Calling exit() is unusual; calling exit(0) to exit successfully is even weirder.

Checking for possible buffer overflow at the end of the function is useless. The damage would already be done, and your program could very well have crashed before reaching the check.

Your use of strlen() and strcat() is inefficient, especially when done in a loop. Each of those calls is an O(n) operation, as it scans the buffer for the \0 terminator. You already know exactly where those terminators are, since you composed the output string yourself.

/**
 * Encodes the input_md5 in ucuc format, whatever that means.
 * The caller must provide an output capable of holding 5 bytes
 * per byte of input, plus 1 byte for a null terminator.
 *
 * Returns the length of the output in bytes, excluding the null
 * terminator.
 */
static int md5_encode(const char *input_md5, char *output)
{
    // Check preconditions
    assert(input_md5);
    assert(output);

    int out_offset = 0;
    for (const char *in = input_md5; *in; in++)
    {
        // Encode it. sprintf() already adds a \0 terminator.
        out_offset += sprintf(output + out_offset, "uc%d", *in);
    }

    return out_offset;
}

In my opinion, two spaces of indentation per level is too stingy for readability, and also encourages inappropriately deep nesting.

StackExchange Code Review Q#41134, answer score: 3