Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't define strlcpy on musl libc #110

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Don't define strlcpy on musl libc #110

merged 1 commit into from
Nov 2, 2021

Conversation

mstemm
Copy link
Contributor

@mstemm mstemm commented Oct 27, 2021

Falco has a build variant that uses musl libc, and musl libc already
defines strlcpy, so we don't want strlcpy when compiling with musl
libc.

Unfortunately, musl doesn't have a define like _MUSL_SOURCE that we
could use to detect when compiling with musl libc. So instead, use
MINIMAL_BUILD, which when set always means compiling with musl libc.

Signed-off-by: Mark Stemm mark.stemm@gmail.com

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area driver-kmod

/area driver-ebpf

/area libscap

/area libsinsp

/area tests

/area proposals

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Don't redefine strlcpy when using musl libc, which is true when building with MINIMAL_BUILD.

@LucaGuerra
Copy link
Contributor

Thanks for this !
Now that I think about it, I believe MacOS, being based on BSD libraries, should have this defined too. Does MINIMAL_BUILD cover that case or should we add a define there?

@gnosek
Copy link
Contributor

gnosek commented Oct 28, 2021

MINIMAL_BUILD is mostly orthogonal to the OS (we can and do have minimal Linux builds). I don't know if strlcpy comes with a matching #define so it will probably be easiest to #ifndef on some mac-specific macro.

leogr
leogr previously approved these changes Oct 28, 2021
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Approved assuming this will make the Falco minimal build again. Anyway, I agree with @gnosek we shouldn't rely on MINIMAL_BUILD for that. Let me know if we find a better solution.

@poiana
Copy link
Contributor

poiana commented Oct 28, 2021

LGTM label has been added.

Git tree hash: 8b8b279f28c1fc4ea40e088de0336398c3e730e3

@mstemm
Copy link
Contributor Author

mstemm commented Oct 28, 2021

Here's the declaration of strlcpy in musl's string.h, from http://git.musl-libc.org/cgit/musl/tree/include/string.h#n81:

#if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
char *strsep(char **, const char *);
size_t strlcat (char *, const char *, size_t);
size_t strlcpy (char *, const char *, size_t);
void explicit_bzero (void *, size_t);
#endif

So musl declares it when either GNU_SOURCE or BSD_SOURCE are defined.

musl also doesn't have its own declaration like __MUSL_SOURCE, see https://wiki.musl-libc.org/faq.html.

So I'm not sure of any explicit define that we could use, other than something we could set when we set MINIMAL_BUILD, which is effectively this anyway.

@deepskyblue86
Copy link
Contributor

I would rather do:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2b7544db..47c33719 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -69,3 +69,10 @@ if(CREATE_TEST_TARGETS AND NOT WIN32)
 			COMMAND ${CMAKE_MAKE_PROGRAM} run-unit-test-libsinsp
 		)
 endif()
+
+include(CheckSymbolExists)
+check_symbol_exists(strlcpy "string.h" HAVE_STRLCPY)
+if(HAVE_STRLCPY)
+	add_definitions(-DHAVE_STRLCPY)
+endif()
+
diff --git a/userspace/common/strlcpy.h b/userspace/common/strlcpy.h
index 7a9f4436..950099d4 100644
--- a/userspace/common/strlcpy.h
+++ b/userspace/common/strlcpy.h
@@ -14,10 +14,13 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+#pragma once
+
 #include <sys/types.h>
 #include <string.h>
 
 
+#ifndef HAVE_STRLCPY
 /*!
   \brief Copy up to size - 1 characters from the NUL-terminated string src to dst, NUL-terminating the result.
 
@@ -40,3 +43,6 @@ static inline size_t strlcpy(char *dst, const char *src, size_t size) {
 
     return srcsize;
 }
+
+#endif
+

No musl:

-- Looking for strlcpy
-- Looking for strlcpy - not found

With export CC="musl-gcc":

-- Looking for strlcpy
-- Looking for strlcpy - found

Falco has a build variant that uses musl libc, and musl libc already
defines strlcpy, so we don't want strlcpy when compiling with musl
libc.

So check for it at cmake time and if found set HAVE_STRLCPY. And only
include the one in strlcpy.h if HAVE_STRLCPY is not defined.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
@mstemm
Copy link
Contributor Author

mstemm commented Nov 1, 2021

I updated the PR to check for strlcpy at cmake time as @deepskyblue86 suggested, please take a look again?

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

LGTM

@deepskyblue86 Thank you for the suggestion!

@poiana poiana added the lgtm label Nov 2, 2021
@poiana
Copy link
Contributor

poiana commented Nov 2, 2021

LGTM label has been added.

Git tree hash: 06162cd850aac5d200d18f1d3e13e02fd0638b30

@gnosek
Copy link
Contributor

gnosek commented Nov 2, 2021

/approve

@poiana
Copy link
Contributor

poiana commented Nov 2, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnosek, mstemm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana added the approved label Nov 2, 2021
@poiana poiana merged commit 078d395 into master Nov 2, 2021
@poiana poiana deleted the no-strlcpy-musl-libc branch November 2, 2021 14:58
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 2, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 2, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 3, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 3, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 5, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 5, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 8, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 8, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 9, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 9, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 9, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 9, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 9, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 12, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 12, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit to falcosecurity/falco that referenced this pull request Nov 12, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
poiana pushed a commit to falcosecurity/falco that referenced this pull request Nov 12, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
sai-arigeli pushed a commit to sai-arigeli/falco that referenced this pull request Nov 16, 2021
Detect strlcpy on the fly, as was done in falcosecurity/libs#110.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants