Documentation is jack_client_name_size is inconsistent with parts of the source code

[milasudril]

According to the documentation jack_client_name_size() returns the maximum size including null terminator. Looking at the source code, it returns JACK_CLIENT_NAME_SIZE, but in JackRequest.cpp:

char fName[JACK_CLIENT_NAME_SIZE + 1];

So either jack_client_name_size() should return JACK_CLIENT_NAME_SIZE + 1, or its documentation should be changed from

the maximum number of characters in a JACK client name including the final NULL character. This value is a constant.

to

the maximum number of characters in a JACK client name excluding the final NULL character. This value is a constant.

[edom]

I suggest that we change the code and keep the documentation so that we can write the following. This way, we don't need the +1, so we have one less thing to remember.

char str[JACK_CLIENT_NAME_SIZE];
// so that sizeof(str) == JACK_CLIENT_NAME_SIZE
// so that we can write strnlen(str, sizeof(str))
// or strnlen(str, JACK_CLIENT_NAME_SIZE) if str is not a char array whose size is known

I also suggest that strlen be replaced with strnlen.

I was going to suggest that strncpy be replaced with snprintf because I thought snprintf ensures that the destination is null-terminated, but it turns out that this is not always the case (#182). It is suggested instead that we stick to C89 and create a wrapper such as snprintf_s (or perhaps even something like jack_snprintf).

The codebase itself is not even consistent about this, as the following partial grep shows.

./common/JackEngineProfiling.h:67:    char fName[JACK_CLIENT_NAME_SIZE + 1];
./common/JackNetTool.h:96:        char fName[JACK_CLIENT_NAME_SIZE];          //slave's name
./common/JackProfiler.cpp:33:        char port_name[JACK_CLIENT_NAME_SIZE + JACK_PORT_NAME_SIZE];
./common/JackNetDriver.cpp:40:            GetHostName(net_name, JACK_CLIENT_NAME_SIZE);
./common/JackNetDriver.cpp:787:                        strncpy(net_name, param->value.str, JACK_CLIENT_NAME_SIZE);
./common/JackProxyDriver.cpp:38:        assert(strlen(upstream) < JACK_CLIENT_NAME_SIZE);
./common/JackNetAdapter.cpp:92:                    strncpy(fParams.fName, param->value.str, JACK_CLIENT_NAME_SIZE);
./common/JackLibClient.cpp:90:    if (strlen(name) >= JACK_CLIENT_NAME_SIZE) {
./common/JackLibClient.cpp:94:                   JACK_CLIENT_NAME_SIZE - 1);
./common/JackRequest.h:1369:            fDst[JACK_CLIENT_NAME_SIZE] = 0;

[7890]

I totally agree that it should be unambiguous if JACK_CLIENT_NAME_SIZE includes the \0 or not, and whether or not jack_client_name_size() tells the raw value of JACK_CLIENT_NAME_SIZE or something else.

This needs to be overhauled to work out the inconsistencies while keeping an eye on not breaking anything that works today and expects exactly the current behavior.

[h1z1]

Anything ever happen with this? It's still biting applications. Someone submitted a patch to ffmpeg for example to bump their buffer to 32 bytes which is still well below what it should be ( and I believe incorrect, should be using jack_port_name_size() )

[falkTX]

These older issues tend to get forgotten about.

afaik there was a PR merged for fixes around JACK_CLIENT_NAME_SIZE, not sure about the state of this now. if there is a change in documentation you want to propose, a pull request / patch would be very welcome.

[h1z1]

Depends what the recommended path is. Not all clients are even handling cases where the name is rejected. Many, including code in this project use static buffers. Kind of shocking how bad some are. Maybe updating examples as a first step reference?