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

Install older Widevine CDM when OS doesn't have TCMalloc support #441

Merged
merged 1 commit into from
May 27, 2021

Conversation

mediaminister
Copy link
Collaborator

@mediaminister mediaminister commented May 25, 2021

With the release of Widevine CDM 4.10.2252.0, Google uses a newer dynamic library that needs an operating system with TCMalloc support and a patched glibc to work. Support for this already implemented in LibreELEC and CoreELEC:
LibreELEC/LibreELEC.tv#5376
CoreELEC/CoreELEC#287

LibreELEC will release a new version supporting the new Widevine CDM: LibreELEC/LibreELEC.tv#5399

This PR tries to handle some problems InputStreamHelper users will run into.

This PR includes:

  • propose the user to install an older Widevine CDM when libtcmalloc_minimal.so is not found.
  • warn the user that Google will remove support for older Widevine CDM's on 31 May, 2021 when installing older CDM's

@mediaminister mediaminister added the arm Related to ARM architecture label May 25, 2021
@mediaminister mediaminister added this to the v0.5.4 milestone May 25, 2021
@mediaminister mediaminister marked this pull request as ready for review May 25, 2021 18:50
@horstle
Copy link
Collaborator

horstle commented May 25, 2021

Looks good, although I'd rather have used the time module, similar to how it's done in update_widevine. timestamp for June 1st would be 1622505600 seconds.

@dagwieers
Copy link
Collaborator

So I think it is problematic that we have the date hardcoded, as we don't know whether (and when exactly) Google will be revoking the old Widevine.

So I would rather release a new package as soon as this has been detected. (And coordinate this with the repo maintainers, @basrieter @enen92 to do this without delay).

Since SCARLET was only updated now, and potentially other devices lag behind, I expect delays. Google can see what the Widevine versions are still out there, and choose a more opportune moment to make the switch.

@basrieter
Copy link
Collaborator

If this is a one time thing for coordinating, no problem for me. We just need to discuss how you update us. I get so much mail from GitHub that just a notification will not work.

@mediaminister
Copy link
Collaborator Author

mediaminister commented May 26, 2021

So I think it is problematic that we have the date hardcoded

Indeed, I'll change this PR to check for libtcmalloc_minimal.so and then ask the user to decide if he wants to install the older hardcoded CDM when TCMalloc is not supported.

@enen92
Copy link

enen92 commented May 26, 2021

Ok to me. I agree with Bas, my gh email isn't even my personal email so I tend to miss stuff. Please use python-external or just DM us on slack

@mediaminister mediaminister changed the title Use hardcoded ChromeOS image before June 1st 2021 Install older Widevine CDM when OS doesn't have TCMalloc support May 26, 2021
@mediaminister mediaminister marked this pull request as ready for review May 26, 2021 12:49
@CastagnaIT
Copy link

CastagnaIT commented May 27, 2021

i think the better thing is check the both places, the file and the kodi.conf, by check in some way the line:
LD_PRELOAD=/usr/lib/libtcmalloc_minimal.so

because an user in case of problems can disable libtcmalloc_minimal by remove line above without rollback

@mediaminister
Copy link
Collaborator Author

Thanks for your suggestion. I'll change the check accordingly.

@mediaminister
Copy link
Collaborator Author

mediaminister commented May 27, 2021

@CastagnaIT
Copy link

i have see that kodiconf_path is hardcoded to libreelec
this check is intended only for Libreelec?

i could try to test on CoreElec but do not think to have time today

@mediaminister
Copy link
Collaborator Author

mediaminister commented May 27, 2021

I have no idea how to check for other operating systems. I guess I better remove this kodi.conf check for the upcoming release. Time is limited and I should get this release out before the weekend.

Users get a yes/no dialog after the check and can decide to install the newer or older Widevine CDM. When installing the older Widevine CDM, users get a warning that Google will remove support.

@sonarcloud
Copy link

sonarcloud bot commented May 27, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mediaminister mediaminister merged commit 3e978df into emilsvennesson:master May 27, 2021
@dagwieers
Copy link
Collaborator

Well done. This is working as expected on my LibreELEC v18.7.

@Portisch
Copy link

Portisch commented Jun 7, 2021

TCMalloc is obsolete and because of random Kodi crashes it got removed.
The problem was fixed properly in glibc dlopen itself.
No user reports so far that it does not work.

So the check for the old/new Widevine lib is not needed anymore.

xbmc/inputstream.adaptive#678 (comment)

@horstle
Copy link
Collaborator

horstle commented Jun 7, 2021

Yes, thanks. We are currently discussing what to do now, since we can't really detect if the user is on a new system with the relevant patches or not.

@Portisch
Copy link

Portisch commented Jun 7, 2021

I think it's ok to remove it and "come back" to normal usage. LibreELEC did merge and remove tcmalloc also as well already.
If a user do have a old image and want to use the new CDM lib he will get a error message in kodi.log.

The correct place to "fix" this will be inputstream.adaptive to evaluate the given dlopen, dlerror error and inform the user he need to update his system or to downgrade CDM lib. If tcmalloc is used or if glibc is patched the error will not happen.

@wagnerch
Copy link

wagnerch commented Jun 7, 2021

@horstle

You could try to dlopen the library in the Python module. Unfortunately they would have to waste the time & bandwidth to download a potentially incompatible version.

#!/usr/bin/env python3
import ctypes

cdm = ctypes.cdll.LoadLibrary("./libwidevinecdm.so")
GetCdmVersion = cdm.GetCdmVersion
GetCdmVersion.restype = ctypes.c_char_p
print(GetCdmVersion().decode('utf-8'))

@Portisch
Copy link

Portisch commented Jun 7, 2021

For this you need to download the new CDM lib and the LoadLibrary will fail if glibc is not compatible.
Only way is to supply a "dummy" library with a TLS alignment of 64 bytes and test dlopen before downloading anything.
By this result the helper can decide if new or old CDM lib is compatible or not.

The other way is to use the check what is already implemented here:
https://github.com/xbmc/inputstream.adaptive/blob/Matrix/wvdecrypter/cdm/base/native_library_posix.cc#L31-L32

When the TLS block error happen at this place the user needs to be informed on screen to downgrade CDM lib or to update the system image to become compatible again.

@wagnerch
Copy link

wagnerch commented Jun 7, 2021

One thing to mention is the issue with newer widevine isn't just TLS alignment, it is also the new section with relative relocations that is unsupported by stock glibc's. I suppose someone could craft a library like this, but the thought process for doing a "dlopen" in Python would at least give InputStream Helper the opportunity to do some recovery logic and download the older CDM, and perhaps save a setting indicating platform capabilities (knowing that this is probably a bad solution when the platform is patched, unless you intend to periodically re-test platform capabilities via 1GB download).

I think probably everyone involved would like to not have a whole bunch of tickets with CDM issues. Pretty sure people will put tickets in for this issue against Kodi, ISA, Netflix, Amazon, and so on, it will just never end. :)

IS Helper is probably the only hope to mitigate the numerous tickets for things that can't be solved by those maintainers.

@dagwieers
Copy link
Collaborator

@Portisch We would like to have the ability to make inputstreamhelper reactive rather than proactive. It would allow us to only attempt updating Widevine CDM when it fails to work (e.g. when revoked) rather than after X days. See #69

@Portisch
Copy link

Sure! As glibc is built from source maybe something can be added to show it's compatible, like something in the versioning or a export of a dummy function or something like this. This should work on LE/CE ARM platform.

@Portisch
Copy link

Just back to the idea about glibc version:
As LE and CE do build glibc from source a string can be added to the glibc version to show if 64 byte alignment is supported or not:
https://sourceware.org/git/?p=glibc.git;a=blob;f=version.h;hb=HEAD

Then check the version like this: ldd --version

@dtechsrv
Copy link

Okay, I see the same topic is under a closed PR.
Unable to detect non-tcmalloc implementation of widevine fix >= 4.10.2252.0 on ARM (LE/CE) #449

@vpeter4
Copy link

vpeter4 commented Jun 16, 2021

One shell script or even one indicator file in /etc could also work. Plain and simple.

@mediaminister
Copy link
Collaborator Author

Then check the version like this: ldd --version

I guess ISH should look for version 2.31.1 or higher?

@mediaminister
Copy link
Collaborator Author

Please check #450

Test packages:
For Kodi 19 Matrix: script.module.inputstreamhelper-0.5.5+matrix.1-tls-04a0795.zip
For Kodi 18 Leia: script.module.inputstreamhelper-0.5.5-tls-04a0795.zip

@vpeter4
Copy link

vpeter4 commented Jun 16, 2021

Why would only version matter? LE/CE is using glibc-2.32 WITH extra 2 patches to allow 64 bytes alignment.
I think glibc version should have some suffix to indicate if 64 bytes alignment is supported or not. Like

# ldd --version
ldd (GNU libc) 2.32-64bytestls

glibc 2.32 itself without patch is not useful.

@Portisch
Copy link

Yes something like this. By side, it's 64 bytes, not bits.
So a version like "2.32-arm64tls" or something like this should do the job. Then the plugin can check if ARCH is arm and if yes check glibc version if compatible with new 64 byte alignment.

@mediaminister
Copy link
Collaborator Author

mediaminister commented Jun 16, 2021

Why would only version matter?

There is only a version to detect in the current LibreELEC and CoreELEC versions. I suffix could help, but this means every single operating system has to add a suffix.

Only way is to supply a "dummy" library with a TLS alignment of 64 bytes

This seems the best solution.
But I currently don't have expert knowledge to build and test such a dummy library. I don't even own ARM hardware myself. It's time consuming for me to achieve this solution.
It would be great if someone can give me a hint or documentation to build this lib.

@Portisch
Copy link

I still think patching the version string is still the easiest. There are already two patches for support the new CDM lib. So just add another one patching the version. Or include it in the 64 byte alignment patch.

Then just parse the version string and the addon know if compatible or not. The patches will never be upstreamed so if anyone else want to add the support he can take this additional patch to.

I can redo the patch tomorrow to include the version string modification.

@CeEs-atd
Copy link

I wonder if you could implement both ways into IS helper:
What if IS helper checks for the "arm64tls" version of glibc and if not successful tries to dlopen an "arm64tlstest.so" library? If one of both results in success IS adaptive will able to load CDM libs with 64 byte TLS on this system.
As soon as someone is able to create an arm64tlstest.so library that is shipped with IS helper, there would be no need to apply the version string patch to glibc any more...
The version patch is a fast fix for OS which follow fast with the glibc patch, the arm64tlstest.so a slower but more sustainable way.

@dagwieers
Copy link
Collaborator

dagwieers commented Jun 16, 2021

I don't think we want to ship a binary in a noarch add-on.

IMO inputstream.adaptive should return proper onPlayBackError information that ISH can act on. See #69 and xbmc/inputstream.adaptive#497

It would make a lot of things easier, like only downloading a newer Widevine if the current one is revoked or fails to get loaded. (So we don't update every X days and possibly break a working setup)

@Portisch
Copy link

I don't think we want to ship a binary in a noarch add-on.

I think the same. This will become a issue.
Here the modified patch and the result how it will look like if applied:

CoreELEC:~ # ldd --version
ldd (GNU libc) 2.32-arm64tls
$Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$Written by Roland McGrath and Ulrich Drepper.
From 1f6f179986e941f8062019fe894996550bcb737a Mon Sep 17 00:00:00 2001
From: Portisch <hugo.portisch@yahoo.de>
Date: Sat, 5 Jun 2021 19:41:25 +0200
Subject: [PATCH] tls: libwidevinecdm.so: since 4.10.2252.0 has TLS with
 64-byte alignment Change the max_align to 64U instead 16 to make it possible
 to use dlopen again. Tests by changing TLS_TCB_ALIGN directly showed up some
 random crashes. Reverence: https://lkml.org/lkml/2020/7/3/754 Modify
 'VERSION' to show TLS 64 byte alignment compatibility.

---
 elf/dl-tls.c | 5 +++++
 version.h    | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/elf/dl-tls.c b/elf/dl-tls.c
index 9fa62f5d..d8f2f740 100644
--- a/elf/dl-tls.c
+++ b/elf/dl-tls.c
@@ -213,6 +213,11 @@ void
 _dl_determine_tlsoffset (void)
 {
   size_t max_align = TLS_TCB_ALIGN;
+  /* libwidevinecdm.so: since 4.10.2252.0 has TLS with 64-byte alignment.
+     Since TLS is initialized before audit modules are loaded and slotinfo
+     information is available, this is not taken into account below in
+     the audit case.  */
+  max_align = MAX (max_align, 64U);
   size_t freetop = 0;
   size_t freebottom = 0;
 
diff --git a/version.h b/version.h
index 83cd1967..2cc5a35a 100644
--- a/version.h
+++ b/version.h
@@ -1,4 +1,4 @@
 /* This file just defines the current version number of libc.  */
 
 #define RELEASE "release"
-#define VERSION "2.32"
+#define VERSION "2.32-arm64tls"
-- 
2.30.0

I know it will need a rebase on every glibc package version bump but it's fixed within seconds.
In my opinion in future a version string check will be needed anyway when (ever) glibc proper fix the dlopen process and when this patch become obsolete.

@dtechsrv
Copy link

dtechsrv commented Jun 17, 2021

In fact, it is not the most beautiful solution, but if it works, it's fine.

I already implemeted it to my repo (LE):
dtechsrv/LibreELEC-AML#3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Related to ARM architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.