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

Features for libmagic API changes #16

Merged
merged 26 commits into from
Oct 22, 2021
Merged

Features for libmagic API changes #16

merged 26 commits into from
Oct 22, 2021

Conversation

robo9k
Copy link
Owner

@robo9k robo9k commented Oct 21, 2021

This adds crate features named "libmagic-abi-v5??" for each change in the v5 libmagic API/ABI.
In general, the API seems to be backwards compatible by only adding new functionality.

The use case for this is supporting both old and new versions of the libmagic API, depending on the user setup.

Currently no effort is made to identify the version of libmagic that is being linked against. If symbols are undefined, a linker error will occur. If linking works, there might still be slight API differences.

It would probably be possible to indirectly identify versions by their symbols and defines, but this seems unclean.
Upstream seems to have switched to autotools and added a #define MAGIC_VERSION in v5.13.
The libmagic.pc was only added in v5.39, see #1

Slightly unrelated changes:

  • all code has been formatted with cargo fmt, no enforcement via CI
  • warnings from cargo clippy have been fixed, no enforcement via CI
  • #defines have mostly been added as constants, so they can be used from e.g. the magic crate
  • cargo test tests were added to ensure linking with the feature gated symbols works
  • the opaque Magic struct has been fixed according to the 'nomicon and now mostly matches what bindgen would emit

Leftover TODOs for myself:

  • Add feature flags documentation in crate readme, possibly configuration for docs.rs
  • libmagic needs to be (any) version 5, update doc for Clarify requirements #5
  • Await CI builds
  • Comment in Add magic_load_buffers #10 which is obsoleted by this merge request here
  • Test integrating new crate in magic crate
  • Publish a new major magic-sys crate release (candidate?)

Misc notes:

  • file-5.00 seems to be from 2009-02-03, that should be sufficiently old to support even ancient systems
  • file-5.41 is the latest considered version from 2021-10-18 and according to e.g. https://repology.org/project/file/badges has not even made it to all package managers
  • the default feature should probably select a little older API version, given that otherwise crate compilation/linkage might fail on most systems currently, see previous bullet point

@robo9k
Copy link
Owner Author

robo9k commented Oct 22, 2021

After staring at that Windows build failure from runs/3969370194 for a while, I believe these are the problems:

magic.lib(magic.obj) : error LNK2019: unresolved external symbol __imp_PathRemoveFileSpecA referenced in function _w32_get_magic_relative_to

Due to the vcpkg triple "x64-windows-static-md" MSVC is instructed to statically build and link "magic.lib". However as can be seen from the error message, that means we need to link to shlwapi for PathRemoveFileSpecA.

Yet when we inspect the output of vcpkg-rs in target\debug\build\magic-sys-*\output:

cargo:rustc-link-lib=magic
cargo:rustc-link-lib=tre
cargo:rustc-link-lib=getopt

Those seem to be coming from microsoft/vcpkg ports/libmagic/vcpkg.json

Looking at the link.exe invocation we also see:

"kernel32.lib"
"advapi32.lib"
"ws2_32.lib"
"userenv.lib"
"msvcrt.lib"

No shlwapi.

The build can be "fixed" by adding the following to build.rs:

println!("cargo:rustc-link-lib=shlwapi");

But that is not the right place.

Looking at file/file src/Makefile.am we see:

if MINGW
MINGWLIBS = -lgnurx -lshlwapi
else
MINGWLIBS =
endif
libmagic_la_LIBADD = $(LTLIBOBJS) $(MINGWLIBS)

The libmagic.pc generated by vcpkg/autotools contains the following:

Libs.private: -ltre -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -lcomdlg32 -ladvapi32

Thus I believe there are 2 upstream issues:

  • file/file would need to fix their Makefile.am to include -lshlwapi for Windows in general, not just MinGW
  • mcgoo/vcpkg-rs would have to use the private libs from pkg-config to build the requested package as a static library

I believe this didn't surface earlier with vcpkg since there was no test with all linked symbols. cargo build does not care about the missing symbols.

@robo9k
Copy link
Owner Author

robo9k commented Oct 22, 2021

According to https://repology.org/project/file/cves the libmagic 5 versions without a CVE at time of writing 2021-10-22 are:

  • 5.38
  • 5.39
  • 5.40
  • 5.41

That might be motivation to drop support for old (API) versions sooner than later.

Packaging status

@robo9k
Copy link
Owner Author

robo9k commented Oct 22, 2021

Regarding feature flag names, see gdesmott/system-deps#18
Essentially they'd have to be named "v*" (instead of "libmagic-abi-v5??").

Feature names could be changed later, but that'd be a semver breaking change.

@robo9k robo9k mentioned this pull request Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant