Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Fix #2777, compiler false positive in xcursor.c #2838

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Apr 10, 2021

This reverts commit 7dffe93, which introduced
another linter error with -O3:

  error: ‘strncat’ specified bound 7 equals source length [-Werror=stringop-overflow=]

and replace strncpy() with memcpy() to suppress the original warning.

This reverts commit 7dffe93, which introduced
another linter error with -O3:

  error: ‘strncat’ specified bound 7 equals source length [-Werror=stringop-overflow=]

This makes sense because strncat(dest, "cursors", strlen("cursors")) is moot
in security point of view.

The next commit will replace strncpy() with memcpy(), so let's restore the
original implementation.
Since len <= strlen(elt) is known, we don't need a str*() function. Let's
simply do memcpy() to suppress linter false positive.

Fixes swaywm#2777.
@kennylevinsen
Copy link
Member

@yuja can you reproduce the original warning to verify that this fixes it? I don't recall experiencing it myself, in which case I'm unable to verify it as fixed.

@yuja
Copy link
Contributor Author

yuja commented Apr 13, 2021

Sorry, I can't reproduce the original truncation warning even with s390x-linux-gnu-gcc-10. Maybe need an s390x VM?
I needed to comment out the path[pathlen + len] = '\0' line to get that, which clearly makes the warning correct.

The use of memcpy() is documented way to suppress the false positive, so I think it would
avoid the problem reported in #2018.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-truncation

@emersion
Copy link
Member

The use of memcpy() is documented way to suppress the false positive

The manual says:

To avoid the warning when the result is not expected to be NUL-terminated, call memcpy instead.

However we expect the result to be NUL-terminated here.

@yuja
Copy link
Contributor Author

yuja commented Apr 13, 2021

we expect the result to be NUL-terminated here.

No, we don't always expect that. Since elt may be a part of comma-separated string (e.g. /usr/local:/usr),
we do append NUL at the end. strncat() would do that automatically, but it introduced another false positive.

@emersion
Copy link
Member

The result is the output of the function, not the input. elt is an input.

@yuja
Copy link
Contributor Author

yuja commented Apr 13, 2021

Yes. I mean elt is partial string, so we can't always make strncpy() copy the NUL byte from elt.
So the result of strncpy() is not expected to be NUL terminated.
That's why we do append NUL.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I see. So the memcpy can never copy past the end of elt.

Thanks for the explanation. This file is terrible.

@emersion emersion merged commit 3c5cc02 into swaywm:master Apr 13, 2021
@emersion emersion mentioned this pull request Apr 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants