AT Client Character Wait Fails Once Ticks Wrap Beyond INT32_MAX

[warasilapm]

Problem Description

The AT client private function uATClientWaitCharacter() does not correctly handle ticks after wrap around.

ubxlib/common/at_client/src/u_at_client.c

Lines 3927 to 3976 in 9694ef9

	stopTimeMs = uPortGetTickTimeMs() + pClient->atTimeoutMs;
	if (stopTimeMs < 0) {
	// Protect against wrapping
	stopTimeMs = pClient->atTimeoutMs;
	}
	while ((errorCode != U_ERROR_COMMON_SUCCESS) &&
	(pClient->error == U_ERROR_COMMON_SUCCESS)) {
	// Continue to look for URCs, you never
	// know when they might turn up
	do {
	// Need to remove any CR/LF's at the start
	while (bufferMatch(pClient, U_AT_CLIENT_CRLF,
	U_AT_CLIENT_CRLF_LENGTH_BYTES, false)) {}
	urcFound = bufferMatchOneUrc(pClient);
	} while (urcFound);
	// Check for a device error landing in the buffer
	deviceErrorInBuffer(pClient);
	// Now we can check for our wanted character, removing
	// at least one character now that we know that what is
	// in there is not a URC. Of course this relies upon
	// the module sending URCs in coherent lines, not
	// stuttering them out with gaps such that we receive just
	// part of a URC prefix, but the alternative is to not
	// remove irrelevant characters (e.g. from URCs that
	// we have set no capture for) in our search for the
	// wanted character, which would be a larger problem
	if (consumeOneCharacter(pClient, character, true)) {
	// Got it: the character will be removed from the buffer
	// and all is good
	errorCode = U_ERROR_COMMON_SUCCESS;
	} else {
	// Remove the processed stuff from the buffer
	bufferRewind(pClient);
	if (pReceiveBuffer->length == 0) {
	// If there's nothing left, try to get more stuff
	if (!bufferFill(pClient, true)) {
	// If we don't get any data within
	// the timeout, set an error to
	// indicate the need for recovery
	setError(pClient, U_ERROR_COMMON_DEVICE_ERROR);
	consecutiveTimeout(pClient);
	} else {
	pClient->numConsecutiveAtTimeouts = 0;
	}
	} else {
	if (uPortGetTickTimeMs() > stopTimeMs) {
	// If we're stuck, set an error
	setError(pClient, U_ERROR_COMMON_DEVICE_ERROR);
	consecutiveTimeout(pClient);

This can lead to two failure cases:

1. If the starting tick is in the range of (INT32_MAX - timeoutMs, INT32_MAX], the check for wrap around causes the timeout to immediately occur.
2. However, if the starting tick is in the range [INT32_MIN, 0], this attempt to check for wrap around instead causes the timeout to only occur when the tick passes timeoutMs again - which could be as long as -INT32_MIN + timeoutMs ticks, or about 25 days.

If the AT server operates correctly, this should not strictly cause a problem. However, a failure during the interval between INT32_MIN and zero which does not provide sufficient characters could take an unexpectedly long time to time out and fail. Additionally, I believe it is possible for the first case to cause a parsing failure in the AT client which would result in a short response length being provided during a subsequent command attempt, triggering the second case.

Suggested Solution

Fix the buggy check to handle wrap around by calculating a difference instead of stop time. See the attached patch.

[RobMeades]

Again, thanks for spotting this, same conclusion/procedure as in #207 applies.

[RobMeades]

The fix for his issue is included in the rather larger fix of commit 79e808e.