fix: support zero-length clientId

[Ryan-CW-Code]

Description

Test Steps

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[Ryan-CW-Code]

https://docs.oasis-open.org/mqtt/mqtt/v3.1.1/errata01/os/mqtt-v3.1.1-errata01-os-complete.html#_Toc442180844

My understanding of this part is that clientId cannot be null but can be a zero-length string.
Asking chat-GPT's response is also like that

[kstribrnAmzn]

My understanding of this part is that clientId cannot be null but can be a zero-length string.

This is true of the actual MQTT connect message itself. The code/library creating that message is free to populate this client identifier anyway it likes while still being compliant.

In several spots in this library we've equated a NULL client id to an empty client id (example) so I tend to agree with @AniruddhaKanhere's opinion. Would you be open to incorporating his suggestions @Ryan-CW-Code?

[kstribrnAmzn]

The other way to think of it: What's the value in erroring when a NULL client ID is provided to the library if it would be invalid to use anyway? This just exposes directly the MQTT specification. Why not use a default like a zero-length identifier and try to connect (aka be useful)?

[Ryan-CW-Code]

I have combined the two erroneous commits into a single commit.

[AniruddhaKanhere]

We should also add this below check since on line connectPacketSize += pConnectInfo->clientIdentifierLength + sizeof( uint16_t ); we are updating the length based only on the length provided by the user without checking whether the value itself is NULL. That will cause an issue.

@Ryan-CW-Code what do you think? I am open to combining the checks too. But I am currently out of office and can help once I am back.

Suggested change

	status = MQTTBadParameter;
	}
	status = MQTTBadParameter;
	}
	else if( ( pConnectInfo->clientIdentifierLength == 0U ) ^ ( pConnectInfo->pClientIdentifier == NULL ) )
	{
	LogError( ( "Client ID length and value mismatch." ) );
	status = MQTTBadParameter;
	}

[kstribrnAmzn]

If we pulled this check before the check on 1709 we could reduce that check too since the XOR implies that if the length is 0 that the value must be NULL.

Not sure I'm sold on this simplification as the code will no longer so obviously equate NULL to 0 length. I like this suggestion.

[AniruddhaKanhere]

I agree with what @kstribrnAmzn said. I was thinking of equating NULL with client ID being 0 length.

I have one minor suggestion though as I was looking through the code. Let me know if that is apt @kstribrnAmzn and @Ryan-CW-Code.

Thanks,
Aniruddha

[Ryan-CW-Code]

The newly added check means that when clientIdentifierLength is 0, pClientIdentifier must be NULL.
NULL is not the same as an empty string "".
Do you agree with this approach?

[kstribrnAmzn]

The newly added check means that when clientIdentifierLength is 0, pClientIdentifier must be NULL.

Yes - and vice versa.

NULL is not the same as an empty string "".

On the surface this is true. If you look deeper into how we've chosen to handle strings in this library then they actually are equal. Let me explain....

A C-string is an array of characters which looks as follow
['k', 's', 't', 'r', 'i', 'b', 'r', 'n', 'A', 'm', 'z', 'n', '\0']

the final character is the null terminator character. This special character is how the end of the string is signalled. This is why you would get kstribrnAmzn when you print the string. If this null terminator was not there, then there would be an out-of-bounds access.

To avoid this problem (and some others that come with it) our library requies you to pass in a string along with a length. This way, if there is no null terminator, there is no problem. If I was to pass the same string above with a length of 12 I'd get back kstribrnAmzn as expected. I can also use a shorter string by passing a length of 8 for example (accesses kstribrn)

Now the trick is how do you represent an empty string? With a C-String it's easy, it's just the null termination character ['\0'] But what about when you don't expect/need the null terminator character? This would be an zero-length character array - which is not allowed in standard C90 - which this library is built against. So the next best representation would be NULL, hence NULL is equivalent to an empty string in what it represents.

Do you agree with this approach?

Please merge in @AniruddhaKanhere's suggestion to your current work. At this point we can merge your CR. Detecting the mismatch as he mentioned is important.

[Ryan-CW-Code]

Sorry, this differs somewhat from my initial expectations. In my understanding, when clientIdentifierLength = 0, passing "" should work correctly.

From my understanding:

• "" is a valid string literal. Although its length is 0, it still points to a valid memory region and contains a terminating '\0'.
• NULL represents a null pointer, meaning the pointer does not refer to any valid object.

Therefore, I cannot agree with treating NULL and "" as equivalent in certain scenarios. They are fundamentally different in semantics:

• "" means “there is a valid string, but it is empty.”
• NULL means “there is no string at all (the pointer is invalid).”

For these reasons, I will temporarily close this PR. Please continue refining the implementation according to your coding practices and project requirements.

Thanks for your patience and understanding.

[kstribrnAmzn]

The way you have your PR written, when clientIdentifierLength = 0, passing "" DOES work correctly. @AniruddhaKanhere's suggestion would break things but this could be change to the following.

else if( ( pConnectInfo->clientIdentifierLength == 0U ) ^ ( ( pConnectInfo->pClientIdentifier == NULL )  || ( *( pConnectInfo->pClientIdentifier ) == '\0' ) ) )
    {
        LogError( ( "Client ID length and value mismatch." ) );
        status = MQTTBadParameter;
    }

Doing this will result in the library treating a Client ID which is empty string OR NULL the same. Which is aligns with our goal.

[Ryan-CW-Code]

Good, I used the code you recommended, and it meets our requirements and the coreMqtt specifications.
Please review it again.
Thank you

[Ryan-CW-Code]

Sorry, this PR can no longer be reopened. Please refer to the new PR。
#331