libarchive / libarchive

Multi-format archive and compression library
http://www.libarchive.org
Other
3.04k stars 770 forks source link

Missing libraries in archive_version_details #2300

Open ljdarj opened 2 months ago

ljdarj commented 2 months ago

I noticed that some libraries such as liblzo and the various ACL and crypto libraries are missing in archive_version_details' output, is that normal?

kientzle commented 2 months ago

A PR to fix this would be appreciated.

evelikov commented 2 months ago

Would be amazing if you can find some way to reminds us so it doesn't get out of sync. Although any help will be welcome.

ljdarj commented 1 month ago

I have two questions, regarding which libraries I should seek the version of: the ARCHIVE_CRYPTO_(algorithm)_(platform) defines (such as ARCHIVE_CRYPTO_RMD160_NETTLE) only denotes what is theoretically possible to use, not what libarchive's build system has decided to use and link, right? And the way to differenciate between the use of CNG and of WinCrypt is, on top of checking ARCHIVECRYPTO(algorithm)_WIN, to check HAVE_BCRYPT_H and _WIN32_WINNT, isn't it?

kientzle commented 1 month ago

The full logic for that is in archive_cryptor_private.h and archive_cryptor.c. Rather than copying that logic, I suggest you instead add ARCHIVE_CRYPTOR_USE_<crypto_library> defines within archive_cryptor_private.h. (Look at how ARCHIVE_CRYPTOR_USE_Apple_CommonCrypto is defined when we know that it should be used.) Basically, this would allow archive_cryptor_private.h to become the only place where we have logic to figure out which encryption library to use.

With that, you can add the common template to archive_version_details() for each crypto library:

   const char *bcrypt = archive_bcrypt_version();

    if (bcrypt != NULL) {
      archive_strcat(&str, " bcrypt/");
      archive_strcat(str, bcrypt_version);
   }

and add the corresponding functions for each library to obtain the version info for that library:

const char *
archive_bcrypt_version(struct archive_string *str)
{
#if ARCHIVE_CRYPTOR_USE_BCRYPT
   /* ...  version info ... */
#else
  return NULL;
#endif
}

You'll also need to add archive_bcrypt_version(), etc, to archive.h so that people can query it. (To be clear, I think we should only provide version info for libraries we actually use. Even if multiple crypto libraries were found on the system, any that we have decided not to use should return NULL here.)

evelikov commented 1 month ago

A cleaner solution IMHO is to have a single API, which takes the component type as enum. Say:

enum libarchive_component [
    PREFIX_BCRYPT,
    PREFIX_MD,
    PREFIX_BZIP,
    ....
};

const char *archive_get_component_version(enum libarchive_component foo);

If gives you fewer entrypoints, easier to document (and read the docs), smaller binaries, etc

ljdarj commented 1 month ago

The one question I have is wouldn't such a function be seen to be about "what library provides functionality X (like, say, RIPEMD160 or ZSTD) if any" rather than the current "is library X linked in and what is its version, if available"?

kientzle commented 1 month ago

The question should be: "What version of library XYZ is libarchive using?" If libarchive is not using it, the answer should be NULL.

We already have the pattern of a separate function for each library. Otherwise, I would agree with @evelikov's idea. But we should stick with the existing pattern.

ljdarj commented 1 month ago

Should I put @evelikov's idea as a 4.0 version API and put the current pattern as deprecated from 4.0 on?

Also, I had a question: when there's a choice between getting the runtime version and the compile-time version of a library like e.g. for Nettle, which one should I use?

kientzle commented 1 month ago

Should I put @evelikov's idea as a 4.0 version API and put the current pattern as deprecated from 4.0 on?

No. API changes should be done only when there's a significant improvement.

... when there's a choice between getting the runtime version and the compile-time version of a library ...

Hmmm.... That's a really good question. 🤔 I think we'd want the runtime version here, since this is a runtime call.