C false positive: Return value of strncpy used in a logical operation.

[tcanabrava]

While auditing some KDE software we got hit by a few false positives.
This one seems like one of them:

https://lgtm.com/projects/g/KDE/okular/snapshot/9755abc39706567915f1d1b757b70e2a0f8e3f3a/files/core/synctex/synctex_parser_utils.c?sort=name&dir=ASC&mode=heatmap#x6d7e052c9ef1e80:1

[rdmarsh2]

I believe that our result is correct there, since all versions of the C specification require that strncpy returns its dest argument unconditionally. Unfortunately, that means there's no way for strncpy to report how much data was copied or whether any errors occurred.

[tsdgeos]

So then you agree that the linked code is correct (even though useless unless using a non conforming c library), if the return of stncpy is not what was passed as dest something went wrong.

[purinchu]

I think that the code is correct, but I'd also not call this alert a 'false positive' in general. I thought it was an error check myself until I looked at the man page for strncpy and saw that is were simply redundant instead, and this alert could catch more serious mistakes.

[tsdgeos]

Sure i'm not say to remove the warning "in general", i'm just saying check if the dest variable is what is being used for comparison, then it's surely not an error of people that thought they could use this for something else

[s0]

if the return of stncpy is not what was passed as dest something went wrong.

In that the implementation of stncpy is incorrect, and not that the copy failed.

To us it still seems as though the linked code is incorrect / dead-code & misleading. As you have said @tsdgeos, the code is useless unless using a non-conforming c library. On initial inspection, the code looks like it's checking for an error (as @purinchu also has said), the fact that it's not is only made clear upon reading the man page for strncpy.

i'm just saying check if the dest variable is what is being used for comparison, then it's surely not an error of people that thought they could use this for something else

I also disagree with this. Even if you're checking against the value of dest, the comparison is always going to either evaluate to true or false, so is dead code. We also have a query that checks for conditions that always evaluate to true or false.

If you want this code to check for an error in the copy, the best thing to do is probably to use strcpy_s instead. (this recommendation can be found in the query help for the alert)

For extra context, you can see the original pull request in which Microsoft contributed this query to the repository.