Rewrite recommendations for the query `cpp/no-space-for-terminator`

[rvermeulen]

This PR aims to improve the recommendations with the following changes:

• Replace segmentation fault with crash that is platform agnostic (I think segmentation fault is not really a thing on Windows).
• Replace security vulnerability with malicious code execution. This provides a range of issues, because a crash (previously segmentation fault) could also be considered a security vulnerability. Namely a DOS.
• Removed the additional note on stack allocated arrays which seem confusing because we are always talking about buffers allocated on the heap.

[github-actions[bot]]

QHelp previews:

cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.qhelp

No space for zero terminator

This rule identifies calls to malloc that call strlen to determine the required buffer size, but do not allocate space for the zero terminator.

Recommendation

The highlighted code segment creates a buffer without ensuring it's large enough to accommodate the copied data. This leaves the code susceptible to a buffer overflow attack, which could lead to anything from program crashes to malicious code execution.

Increase the size of the buffer being allocated by one or replace malloc, strcpy pairs with a call to strdup

Example

void flawed_strdup(const char *input)
{
	char *copy;

	/* Fail to allocate space for terminating '\0' */
	copy = (char *)malloc(strlen(input));
	strcpy(copy, input);
	return copy;
}

References

• CERT C Coding Standard: MEM35-C. Allocate sufficient memory for an object.
• Common Weakness Enumeration: CWE-131.
• Common Weakness Enumeration: CWE-120.
• Common Weakness Enumeration: CWE-122.

[jketema]

Removed the additional note on stack allocated arrays which seem confusing because we are always talking about buffers allocated on the heap.

This is not correct. alloca is also covered by this query. However, I'm fine with removing this remark.