Difference between revisions of "Coding style/Practices"

Revision as of 13:03, 31 October 2008

String Handling

Code should not use any of the following functions: strcpy, strcat, sprintf, or any *scanf function.

Dynamically allocated buffers are preferred to fixed-sized buffers. When using dynamic buffers:

• Use strdup or asprintf for simple constructions.
• Use the k5buf module for complex constructions. If this is not desirable, strlcpy and strlcat are valid alternatives.

When using fixed-sized buffers:

• Use strlcpy for simple copies.
• Use snprintf for simple compound constructions. Avoid using precision or field width specifiers in snprintf format strings.
• To check for truncation when using snprintf, use the following approach:

result = snprintf(buffer, sizeof(buffer), "format string", ...);
if (SNPRINTF_OVERFLOW(result, sizeof(buffer))
    handle_overflow_error;

The SNPRINTF_OVERFLOW macro is defined in k5-platform.h.

• Use the k5buf module for complex constructions. If this is not desirable, strlcpy and strlcat are valid alternatives.

Current conformance

Existing code does not conform, but is under active conversion.

Rationale

It is relatively common to audit a code base such as krb5 and flag all uses of strcpy and similar functions, even if they are used safely. Verifying that the function is used safely requires manual inspection. Using safer alternatives does not guarantee code safety, but does reduce the likelihood of a catastrophic buffer overflow vulnerability.

In some compilation environments under Solaris, field widths and precisions are computed using screen columns rather than bytes.

sprintf returns a signed integer; buffer sizes are typically size_t (which is an unsigned type). Comparing the two directly will result in a warning from some compilers, and has unpredictable semantics depending on the relative widths of int and size_t. Also, some sprintf implementations, such as the one in Solaris prior to version 10, return -1 on a buffer overflow, whereas the C99 behavior returns the number of bytes which would have been written to the string. The SNPRINTF_OVERFLOW macro uses casts to address both issues.

The k5buf module safely allows multi-step string construction within fixed-size or dynamic buffers without performing an error check on each append, and without pre-computing lengths. Errors need only be checked when the resulting string needs to be read or handed off to another function.

Exception Handling

If a function allocates several resources and has many exit paths, use the following general structure when possible:

{
  krb5_error_code retval;
  char *ptr1 = NULL;
  krb5_blah *ptr2 = NULL;

...
  retval = another_function(...);
  if (retval != 0)
    goto cleanup;
...
cleanup:
  if (ptr1)
    free(ptr1);
  if (ptr2)
    kbr5_free_blah(ptr2);
  return retval;
}

Simpler functions may use simpler structures, but do not get to the point of freeing the same resource in several different places within a function depending on the exit path.

Current conformance

Existing code mostly conforms, with some exceptions.

Rationale

This is the most reliable way to avoid resource leaks within the constraints of the krb5 code base. Within libraries, the code base does not abort on memory allocation failure, so tends to have many exit paths within some functions.