Don't overflow callsign buffer in QTC parsing

[df7cb]

strncpy must copy at most one less byte than what fits into the destination buffer, so we can properly NUL-terminate it.

While at it, introduce a proper constant for the size of the buffer.

[dl1jbe]

Two questions:

• Did you check that 14 characters are enough for the longest callsign? Or should we extend the buffer by one.
• Who does the NUL-termination for the buffer? Maybe use g_strlcpy instead which guarantees NUL-termination.

[airween]

I added a question, please take a look at that. Btw it's a nice catch, thanks!

[df7cb]

Since the old buffer was only 15 bytes, and no one complained about problems, I figured that everyone was happy with 14 characters in practice. We could resize the buffer, though.

NUL-termination is done a few lines down in https://github.com/Tlf/tlf/pull/438/files#diff-3fa5e7b67061091b07b1e965f8b50fe4fe7e4824ba82aabdafeb0be7a71722b3R162, though perhaps that != ' ' loop isn't quite safe (I guess that limits the effective call size to 13 so there's still place for a space).

I removed the duplicated macro. (It turned out that including qtcvars.h there is enough, apparently that solution was too complicated for me yesterday night 🥱 )

[airween]

LGTM

[airween]

though perhaps that != ' ' loop isn't quite safe (I guess that limits the effective call size to 13 so there's still place for a space).

Before that check callsign filled with chars from logline - which contains the callsign and spaces. I think if the log is not truncated then it's good. But yes, we can add some more check if you think.

I removed the duplicated macro. (It turned out that including qtcvars.h there is enough, apparently that solution was too complicated for me yesterday night 🥱 )

No worries, I'm sure everyone was in same situation 😄.

Thanks for contributing!

@dl1jbe Thomas feel free to merge if you think everything is okay.

[dl1jbe]

though perhaps that != ' ' loop isn't quite safe (I guess that limits the effective call size to 13 so there's still place for a space).

Before that check callsign filled with chars from logline - which contains the callsign and spaces. I think if the log is not truncated then it's good. But yes, we can add some more check if you think.

If we assume a long callsign with 14 characters it may happen that the while loop terminates way beyond the callsign field and then the \0 goes out of bounds.

I think we should fix these problem while we at it now. Limiting the number of tests (for{} loop instead of while{}) or using max(i, QTC_CALL_SIZE-1) could do. Other way would be to zero out the buffer before and copy characters until ' ' character found or limit reached.

[airween]

I think we should fix these problem while we at it now. Limiting the number of tests (for{} loop instead of while{}) or using max(i, QTC_CALL_SIZE-1) could do. Other way would be to zero out the buffer before and copy characters until ' ' character found or limit reached.

May be a new condition would be enough here:

    while (i < QTC_CALL_SIZE && callsign[i] != ' ') {

(I intentionally put the new condition forward.)

[dl1jbe]

May be a new condition would be enough here:

    while (i < QTC_CALL_SIZE && callsign[i] != ' ') {

That was what I meant by 'limiting the number of tests'. But shouldn't it be 'i < QTC_CALL_SIZE - 1' to keep the index inside the field?

[airween]

That was what I meant by 'limiting the number of tests'.

ah, thanks!

But shouldn't it be 'i < QTC_CALL_SIZE - 1' to keep the index inside the field?

You're right, -1 is necessary there.

@df7cb do you want to add this new condition to this patch?

[dl1jbe]

@df7cb do you want to add this new condition to this patch?

Seems not so. So please see the code at https://github.com/Tlf/tlf/tree/qtc_call_size

That should do it.

@airween BTW, we allow 'SEND', 'RECV' and 'SEND|RECV' for qtcdirection, but in more than one case we check only for 'direction == SEND' or 'direction == RECV'. That would exclude the case when both are set simultaneously.

Can you please check and verify that the code is correct in these cases.

[airween]

Seems not so. So please see the code at https://github.com/Tlf/tlf/tree/qtc_call_size

Then I suggest merge this PR and then I'm going to add that fix too.

@airween BTW, we allow 'SEND', 'RECV' and 'SEND|RECV' for qtcdirection, but in more than one case we check only for 'direction == SEND' or 'direction == RECV'. That would exclude the case when both are set simultaneously.

Can you please check and verify that the code is correct in these cases.

I can take a look it later - thanks for letting know this.

[dl1jbe]

Well. I'll merge it and add the fix for the loop termination afterwards.

Anyway thanks for the PR to @df7cb.

[df7cb]

Thanks for merging, and sorry for the lack of earlier feedback, my TODO list is packed unhealthily dense.