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

Fix build errors with multiple Lua libraries #2718

Open
wants to merge 1 commit into
base: v3/master
Choose a base branch
from

Conversation

stanhu
Copy link

@stanhu stanhu commented Apr 19, 2022

This commit fixes two items:

  1. Previously configure would search for Lua in known paths, but
    this could cause the Makefile to use inconsistent header and library
    paths. For example, if luajit were installed in /usr/local/include
    but lua 5.1 were installed in /usr/lib/liblua-5.1.so, the build
    would attempt to use the luajit headers but link against the lua 5.1
    library.

To fix this, we switch the order of the search:

  • First attempt to find an installed LUA library with pkg-config.
  • If we cannot find a library that way, fall back to the known-path
    scan.

This is actually what is already documented in LUA_POSSIBLE_PATHS.

Note that PKG_CONFIG_PATH can be specified in configure to look in the right path:

PKG_CONFIG_PATH=/usr/local/pkgconfig ./configure
  1. Add luajit back into LUA_POSSIBLE_LIB_NAMES. This was added in
    0ac23a4 but quietly reverted in fe98ce4. The changes in the first
    item also ensure the CFLAGS are set properly for luajit.

This should fix the issues raised in #1909.

@stanhu
Copy link
Author

stanhu commented Apr 20, 2022

@victorhora Could you take a look at this?

@stanhu
Copy link
Author

stanhu commented Jul 1, 2022

@martinhsv Would you be able to review this?

@martinhsv
Copy link
Contributor

Hello @stanhu ,

The only question that comes to mind initially is about where you have placed the two new items in LUA_POSSIBLE_LIB_NAMES.

I guess I'm wondering aloud if it might be better for the two luajit names to be at the end of the list instead. That might make the order in which things are looked for more straightforward (I.e. all regular lua options first, then luajit)

@stanhu stanhu force-pushed the sh-use-pkgconfig-first-add-luajit branch from 048a591 to f514740 Compare October 14, 2022 21:42
@stanhu
Copy link
Author

stanhu commented Oct 14, 2022

@martinhsv I moved them to the end. It's been a while since I looked at this, so I don't remember if there was a reason why I put them before the standard lua options. But the main issue as I recall was that we weren't taking advantage of pkg-config to find the right libraries and headers when there was ambiguity.

@stanhu
Copy link
Author

stanhu commented Oct 14, 2022

From #1909 (comment), I think I intended to the luajit modules to precede lua since luajit could install itself in /usr/local/lib/lua.

This commit fixes two items:

1. Previously `configure` would search for Lua in known paths, but
this could cause the Makefile to use inconsistent header and library
paths. For example, if luajit were installed in `/usr/local/include`
but lua 5.1 were installed in `/usr/lib/liblua-5.1.so`, the build
would attempt to use the luajit headers but link against the lua 5.1
library.

To fix this, we switch the order of the search:

* First attempt to find an installed LUA library with `pkg-config`.
* If we cannot find a library that way, fall back to the known-path
scan.

This is actually what is already documented in `LUA_POSSIBLE_PATHS`.

2. Add luajit back into `LUA_POSSIBLE_LIB_NAMES`. This was added in
0ac23a4 but quietly reverted in fe98ce4. The changes in the first
item also ensure the `CFLAGS` are set properly for luajit.

This should fix the issues raised in owasp-modsecurity#1909.
@stanhu stanhu force-pushed the sh-use-pkgconfig-first-add-luajit branch from f514740 to 9acd950 Compare October 14, 2022 21:59
@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants