Semmle 1.20
Skip to end of metadata
Go to start of metadata

Name: Suspicious call to memset

Description: Use of memset where the size argument is computed as the size of some non-struct type. When initializing a buffer, you should specify its size as <number of elements> * <size of one element> to ensure portability.

ID: cpp/suspicious-call-to-memset

Kind: problem

Severity: recommendation

Precision: medium

Query: SuspiciousCallToMemset.ql
/**
 * @name Suspicious call to memset
 * @description Use of memset where the size argument is computed as the size of
 *              some non-struct type. When initializing a buffer, you should specify
 *              its size as <number of elements> * <size of one element> to ensure
 *              portability.
 * @kind problem
 * @id cpp/suspicious-call-to-memset
 * @problem.severity recommendation
 * @precision medium
 * @tags reliability
 *       correctness
 *       security
 *       external/cwe/cwe-676
 */
import cpp

/**
 * Holds if `e` is a `sizeof` expression on type `t`, with
 * optional multiplication by a constant.
 */
predicate sizeOfExpr(Expr e, Type t) {
  (
    t = e.(SizeofTypeOperator).getTypeOperand()
  ) or (
    t = e.(SizeofExprOperator).getExprOperand().getType()
  ) or (
    sizeOfExpr(e.(MulExpr).getAnOperand(), t) and
    e.(MulExpr).getAnOperand() instanceof Literal
  )
}

/**
 * Gets the type `t` with typedefs, array types and references removed.
 * 
 * This is similar to `Type.stripType` except that it doesn't remove
 * a `PointerType`.
 */
Type stripType(Type t) {
  result = stripType(t.(TypedefType).getBaseType()) or
  result = stripType(t.(ArrayType).getBaseType()) or
  result = stripType(t.(ReferenceType).getBaseType()) or
  result = stripType(t.(SpecifiedType).getBaseType()) or
  result = stripType(t.(Decltype).getBaseType()) or
  (
    not t instanceof TypedefType and
    not t instanceof ArrayType and
    not t instanceof ReferenceType and
    not t instanceof SpecifiedType and
    not t instanceof Decltype and
    result = t
  )
}

/**
 * Holds if `t` points to `base` via a specified number of levels of pointer
 * indirection.  Intermediate typedefs and array types are allowed. Note that
 * `base` is a stripped type (via `stripType`).
 */
predicate pointerIndirection(Type t, int indirection, Type base) {
  (
    base = stripType(t) and
    not base instanceof PointerType and
    indirection = 0
  ) or (
    pointerIndirection(stripType(t).(PointerType).getBaseType(), indirection - 1, base)
  )
}

/**
 * Holds if `t` points to a non-pointer, non-array type via a specified number
 * of levels of pointer indirection.  Intermediate typedefs and array types are
 * allowed.
 */
predicate pointerIndirection2(Type t, int indirection) {
  (
    not stripType(t) instanceof PointerType and
    indirection = 0
  ) or (
    pointerIndirection2(stripType(t).(PointerType).getBaseType(), indirection - 1)
  )
}

/**
 * Holds if `memset(dataArg, _, sizeArg)`, where `sizeArg` has the form
 * `sizeof(type)`, could be reasonable.
 */
predicate reasonableMemset(FunctionCall fc) {
  exists(Expr dataArg, Expr sizeArg |
    dataArg = fc.getArgument(0) and sizeArg = fc.getArgument(2) and
    exists(Type dataType, Type sizeOfType |
      dataType = dataArg.getType() and
      sizeOfExpr(sizeArg, sizeOfType) and
      exists (int i |
        exists(Type base |
          // memset(&t, _, sizeof(t))
          pointerIndirection(dataType, i + 1, base) and
          pointerIndirection(sizeOfType, i, base)
        ) or exists(Type base |
          // memset(t[n], _, sizeof(t))
          pointerIndirection(dataType.getUnspecifiedType().(ArrayType), i, base) and
          pointerIndirection(sizeOfType, i, base)
        ) or exists(VoidType vt |
          // memset(void *, _, sizeof(t))
          pointerIndirection(dataType, i + 1, vt) and
          pointerIndirection2(sizeOfType, i)
        ) or exists(Type ct |
          // memset(char *, _, sizeof(t)) and similar
          ct.getSize() = 1 and
          pointerIndirection(dataType, i + 1, ct) and
          pointerIndirection2(sizeOfType, i)
        ) or exists(Type ct |
          // memset(char [], _, sizeof(t)) and similar
          ct.getSize() = 1 and
          pointerIndirection(dataType.getUnspecifiedType().(ArrayType), i, ct) and
          pointerIndirection2(sizeOfType, i)
        )
      )
    )
  )
}

from FunctionCall fc, Type t
where
  fc.getTarget().hasName("memset") and
  sizeOfExpr(fc.getArgument(2), t) and
  not reasonableMemset(fc)
select fc, "The size of the memory area set by memset should not be the size of the type " + t.getName() + "."

This rule finds calls to the standard library function memset where the third argument (specifying the number of bytes to set) does not appear to correspond with the first argument (the buffer to be written).

Usually, memset is employed to initialize dynamically allocated arrays or structs. Since memset treats its first argument simply as an array of bytes, the third argument has to specify the size of the buffer in bytes. For an array, the size is the number of elements of the array multiplied by the size of one of its elements; for a struct, the size is just the size of the struct type.

If memset is invoked with a third argument that is not constant and looks like neither of these two cases, there might be a mistake. This could cause a buffer overflow which could in turn cause a segfault or corrupt the contents of other variables in memory.

Recommendation

For structs the preferred way of computing the size is to use sizeof with the type as the argument. Dereferencing a pointer works but is more prone to mistakes. For arrays the best solution is to take the size of the array rather than the type, since the risk of forgetting to multiply with the number of elements is eliminated. Do not use memset to assign to scalars or pointers when a simple assignment would do.

Example

// correct, memset uses sizeof(type)
int is[10];
memset(is, 0, sizeof(is));
struct S s;
memset(&s, 0, sizeof(struct S));

// incorrect examples
struct T *t1 = (struct T*)malloc(sizeof(struct T));
struct T *t2 = (struct T*)malloc(sizeof(struct T));
// the size of the struct is probably intended
// but this takes the size of a pointer
memset(t2, 0, sizeof(t2));

// correct but discouraged, use sizeof(struct T) instead
memset(t1, 0, sizeof(*t2));
// correct, but it is preferred to do a direct assignment, i.e., t = 0;
memset(&t2, 0, sizeof(t2));

References