Skip to content

Conversation

@dscho
Copy link
Member

@dscho dscho commented Sep 24, 2025

Currently, CodeQL mis-identifies a couple instances of the usually-legitimate "User-controlled data may not be null terminated" problem.

In these instances, CodeQL thinks that packet_txt_read() fails to NUL-terminate (not actually "null"... tsk, tsk) the string. But it totally does! Here is the current definition of that function:

size_t packet_txt_read(char *buf, size_t count, FILE *stream)
{
	size_t len;

	len = packet_bin_read(buf, count, stream);
	if (len && buf[len - 1] == '\n')
	{
		len--;
	}

	buf[len] = 0;
	return len;
}

The buf[len] = 0 statement guarantees that the string is NUL-terminated.

Currently, CodeQL mis-identifies a couple instances of the
usually-legitimate "User-controlled data may not be null terminated"
problem.

In these instances, CodeQL thinks that `packet_txt_read()` fails to
NUL-terminate (not actually "null"... tsk, tsk) the string. But it
totally does! Here is the current definition of that function:

	size_t packet_txt_read(char *buf, size_t count, FILE *stream)
	{
		size_t len;

		len = packet_bin_read(buf, count, stream);
		if (len && buf[len - 1] == '\n')
		{
			len--;
		}

		buf[len] = 0;
		return len;
	}

The `buf[len] = 0` statement guarantees that the string is
NUL-terminated.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho requested a review from mjcheetham September 24, 2025 14:57
@dscho dscho self-assigned this Sep 24, 2025
@dscho dscho enabled auto-merge September 24, 2025 14:59
Copy link
Member

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buf[len] = 0;

I wonder if CodeQL is being stupid and expects to see buf[len] = NUL; instead... 🤦‍♂️

Either way, this suppression seems reasonable.

@dscho dscho merged commit 4bd1703 into master Sep 24, 2025
53 checks passed
@dscho
Copy link
Member Author

dscho commented Sep 24, 2025

I wonder if CodeQL is being stupid and expects to see buf[len] = NUL; instead... 🤦‍♂️

I guess so, but I'd expect it to expect '\0'. Same problem, though, it's just inviting false positives.

@dscho dscho deleted the suppress-codeql-false-positive branch September 24, 2025 20:12
@mjcheetham mjcheetham mentioned this pull request Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants