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

Rename yara str functions to avoid symbol collisions #6917

Conversation

Smjert
Copy link
Member

@Smjert Smjert commented Jan 25, 2021

Yara publicly exposes the definition of various str functions like
strlcpy, strlcat and so on if they are not present on the system
it is compiled on.
This is not ideal because other libraries use custom implementations
of those functions and those symbols would collide with
the public ones from yara, therefore we rename them
to avoid the collision.

Yara publicly exposes the definition of various str functions like
strlcpy, strlcat and so on if they are not present on the system
it is compiled on.
This is not ideal because other libraries use custom implementations
of those functions and those symbols would collide with
the public ones from yara, therefore we rename them
to avoid the collision.
@Smjert Smjert added build libraries For things referring to osquery third party libraries cmake pure cmake changes labels Jan 25, 2021
@theopolis
Copy link
Member

Has the upstream tip for YARA changed these? Should we also send them an issue / heads up to rename the functions?

@Smjert
Copy link
Member Author

Smjert commented Jan 25, 2021

Upstream haven't changed these, actually there are more defines messing with other libc functions...
I think they are assuming that the implementation, when and if found, is equivalent no matter where it comes from.

Admittedly this is a bit of a lazy way to fix this; a better way would be to have a library that provides those functions in one place, and then tell Yara and the other libraries that we have an implementation they can use.

I was briefly looking into Gnulib, but I was having a hard time understanding how to use their tool to generate the source code for the few functions I needed (was experimenting on building the toolchain with musl, which misses some functions that LLVM needs).

@mike-myers-tob
Copy link
Member

I think they are assuming that the implementation, when and if found, is equivalent no matter where it comes from.

It's not an entirely valid assumption, though (see 6.2.3 about the Solaris versus OpenBSD implementations)

Admittedly this is a bit of a lazy way to fix this; a better way would be to have a library that provides those functions in one place, and then tell Yara and the other libraries that we have an implementation they can use.

Since they are (unfortunately) non-standard, I don't know if maintaining another library is any better than maintaining this patch of libyara.

How about this? https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strlcpy

@Smjert
Copy link
Member Author

Smjert commented Jan 26, 2021

It's not an entirely valid assumption, though (see 6.2.3 about the Solaris versus OpenBSD implementations)

Well, I would expect the library to be based on a common implementation of strlcpy, which is the OpenBSD one (and indeed looking at the replacement that the library itself proproses, is taken from OpenBSD).
There's also libmagic and lldpd which need (and provide) a strlcpy/strlcat implementation which are both taken again from OpenBSD.

Since they are (unfortunately) non-standard, I don't know if maintaining another library is any better than maintaining this patch of libyara.

How about this? https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strlcpy

To be fair the objective was to commit as few source files as possible so that we don't have to maintain the configuration of a complex library.
Maybe at this point, given that each library ships with the same copy of strlcpy and friends, we could just make our own copy in our own library and share it.
Obviously I'll check that each library that requires a strlcpy or strlcat do not expect any weird implementation, but it doesn't seems likely.

@mike-myers-tob
Copy link
Member

Since they are (unfortunately) non-standard, I don't know if maintaining another library is any better than maintaining this patch of libyara.
How about this? https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strlcpy

To be fair the objective was to commit as few source files as possible so that we don't have to maintain the configuration of a complex library.
Maybe at this point, given that each library ships with the same copy of strlcpy and friends, we could just make our own copy in our own library and share it.
Obviously I'll check that each library that requires a strlcpy or strlcat do not expect any weird implementation, but it doesn't seems likely.

No I agree, I wouldn't suggest another library dependency just for this, if a patch to rename the routines in libyara will suffice. I just noted that glib has a portability wrapper around these routines, before realizing that glib probably doesn't solve the problem for Windows or macOS.

@Smjert
Copy link
Member Author

Smjert commented Jan 26, 2021

I just noted that glib has a portability wrapper around these routines, before realizing that glib probably doesn't solve the problem for Windows or macOS.

I'm not sure what portability problems you're thinking about?
This the implementation they use in glib:
https://github.com/GNOME/glib/blob/5339082bbb33b173eb9878dc32f27b91b559cd4e/glib/gstrfuncs.c#L1421-L1454

This is the same implementation that libmagic, lldpd and Yara use, and there's only that implementation for all platforms.

No I agree, I wouldn't suggest another library dependency just for this, if a patch to rename the routines in libyara will suffice.

But actually I would add a lib if it consisted only of those functions and nothing else :D. Although I realized now that gnulib doesn't have the implementation for those functions.

So, we can accept this patch for now, but I don't think it makes sense to keep N implementations of the same functions, so ideally we would just add a new "thirdparty" library which we "maintain", which contains a copy of those functions, then that library is used by all the other ones that need those non-standard functions, so that there's only one implementation in the end and it's controlled by us.

@theopolis theopolis merged commit 776eba9 into osquery:master Jan 27, 2021
@mike-myers-tob mike-myers-tob deleted the stefano/libraries/yara-duplicate-str-functions branch February 18, 2021 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build cmake pure cmake changes libraries For things referring to osquery third party libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants