Bundled libstdc++ should NOT be loaded for binaries outside of the AppImage about linuxdeploy-plugin-checkrt HOT 16 CLOSED

Isn't this exactly what exec.so should fix? cc @pamarcos

from linuxdeploy-plugin-checkrt.

Yes, that's exactly what's meant to do according to the nice comment: https://github.com/darealshinji/AppImageKit-checkrt/blame/master/exec.c#L21-L40

I haven't tested it yet, though. I had a similar error when opening Firefox and I fixed it (before I discovered exec.so I was only adding the patched AppRun ) by removing the APPDIR parts from PATH, LD_LIBRARY_PATH and LD_PRELOAD while maintaining them for calling processes inside the AppImage. Basically, I did what exec.so does but on my own application in C++ because it was more convenient for me. It's a workaround if you can edit the environment before invoking processes in your application.

I think independently of whether we add to LD_LIBRARY_PATH the optional libstdc++ or libgcc_s, we should still add exec.so to LD_PRELOAD. This way, we remove everything that the original AppRun adds to the env variables that may also mess with external processes. That basically means getting rid of the if condition at https://github.com/darealshinji/AppImageKit-checkrt/blob/master/checkrt.c#L106-L111. I even wonder whether this should actually be included in the original AppRun rather than here, for the same reason stated before.

I'll take a look at this tomorrow and reimplement the logic if needed in plain C in exec.c for consistency.

EDIT: I took a quick look and I don't really understand why it's using APPIMAGE_ORIG_PREFIX and APPIMAGE_STARTUP. I thought we simply needed to remove the AppDir parts of every env variable for outside-of-AppImage processes since those are introduced by AppRun and cause trouble. @darealshinji could you explain the reasoning behind that?

from linuxdeploy-plugin-checkrt.

I think that APPIMAGE_ORIG_PREFIX code is from Sven Brauch (KDevelop team) actually. He tried to explain it to me one night but it was way over my head.

from linuxdeploy-plugin-checkrt.

I will try to contact him. In any case, this is the error I get when trying to open Firefox from within my AppImage when I don't clear the env variables internally:

APPIMAGE_CHECKRT>> /usr/lib/x86_64-linux-gnu/libstdc++.so.6 ==> GLIBCXX_3.4.24 (24)
APPIMAGE_CHECKRT>> ./optional/libstdc++/libstdc++.so.6 ==> GLIBCXX_3.4.24 (24)

APPIMAGE_CHECKRT>> optional_ld_library_path: 
optional_ld_preload: (null)
XPCOMGlueLoad error for file /usr/lib/firefox/libmozgtk.so:
/usr/lib/x86_64-linux-gnu/libpangoft2-1.0.so.0: undefined symbol: hb_font_funcs_set_variation_glyph_func
Couldn't load XPCOM.

Notice that both libstdc++ versions are the same, so nothing is actually appended to LD_LIBRARY_PATH or LD_PRELOAD. Hence, the checkrt logic shouldn't be the one causing this issue. In fact, the moment I remove the LD_LIBRARY_PATH from my app before launching Firefox, it works just fine.

from linuxdeploy-plugin-checkrt.

I already talked with Sven. Seems the intended usage is as follows. You need to setup 2 sets of variables before running your app: the ones that should be used for internal processes (APPIMAGE_STARTUP) and the ones for external processes (APPIMAGE_ORIGINAL).

Here's an example of how they do it: https://paste.kde.org/p0bwvuuk1

In that sense, AppRun may do the same after injecting the exec wrapper: set APPIMAGE_ORIGINAL_{VAR} to its original value and then APPIMAGE_STARTUP_{VAR} for the modified version. However, I think it's simpler if we have a different exec wrapper that simply removes the APPDIR from the env variables for internal processes.

What do you think @probonopd, @TheAssassin?

from linuxdeploy-plugin-checkrt.

Doesn't that kind-of spam the environment? And how to deal with future variables which might be modified by the runtime? Do we have to introduce a "runtime compatibility version" like thing that is incremented on changes, or would you add tests for all possible APPIMAGE_ORIGINAL_ variables?

from linuxdeploy-plugin-checkrt.

We are only spamming the environment if we take the same approach taken by them with that exec wrapper. That's exactly why I said that I think it's simpler and more convenient to just parse all env variables and remove all the APPDIR references.

Easy to maintain. There's no need to be updated in case we add or modify a new variable.

from linuxdeploy-plugin-checkrt.

think it's simpler if we have a different exec wrapper that simply removes the APPDIR from the env variables for internal processes

I do not understand what you mean by "remove all the APPDIR references". The issue that leads to crashes is not the APPDIR environment variable but especially LD_LIBRARY_PRELOAD (which in this particular case loads the bundled libstdc++.so.6).

So I think we should do the approach that Kdevelop uses: Ensure that whatever environment variables we set (such as LD_LIBRARY_PRELOAD) get reset to the "before AppRun" state for executables outside the AppImage that get launched from within the AppImage.

In other words, we need to limit the scope of our modifications to environment variables that we set in runtime.c and/or AppRun (first and foremost, LD_LIBRARY_PRELOAD) to executables launched from within the AppImage.

Does that make sense?

from linuxdeploy-plugin-checkrt.

I do not understand what you mean by "remove all the APPDIR references". The issue that leads to crashes is not the APPDIR environment variable but especially LD_LIBRARY_PRELOAD (which in this particular case loads the bundled libstdc++.so.6).

Let me clarify what I meant. When I say APPDIR references I do not refer to the APPDIR env variable. I refer to the APPDIR paths within any other env variable. So, instead of having 2 sets of environments that would spam the environment the same way the KDE's exec wrapper is doing, our exec wrapper for AppImage should hook the exec as well and loop through all env variables to remove the APPDIR paths from them.

The pseudocode would be something like:

new_env = {}
for var : old_env
    new_var = var
    new_var.value.remove(APPDIR + ":")
    new_env.put(new_var)
exec(.., new_env)

As I said, I think this should be done in the original AppImage itself at AppImage/AppImageKit#616, because this particular issue you have is probably due to libstdc++, but there are many other uses we may have due to some other libs.

from linuxdeploy-plugin-checkrt.

What if the AppImage contains an AppRun script that does something like cd $HERE; export LD_LIBRARY_PATH=./usr/lib/ or even LD_LIBRARY_PATH=.? I think the best way would be to somehow restore the pre-AppImage environment (can we get at it using /proc/xxx/env?) for outside applications being launched by applications inside the AppImage.

from linuxdeploy-plugin-checkrt.

I see. As a rule of thumb, I'd say people should never use relative paths for these env variables, but I get your point. Can we agree then on the following mechanism to restore env variables?

1. Save the value of env variables modified by AppRun. Or should we save just everything to make sure the whole status is the same? Restoring every env variable basically disables the ability of an external app to run with a different environment than the one AppRun had at startup
2. Create an exec wrapper that will be always injected through LD_PRELOAD
3. That exec wrapper will restore the original env variables saved at 1 before calling exec

As I have said, I think we should do this directly in the upstream AppImageKit repo: AppImage/AppImageKit#616

from linuxdeploy-plugin-checkrt.

As a rule of thumb, I'd say people should never use relative paths for these env variables

In many cases (e.g., when you want to binary-patch an application for which the source is either unavailable or re-compiling would be a hassle) using relative paths is actually the most straightforward way.

That exec wrapper will restore the original env variables saved at 1 before calling exec

Is retrieving them from /proc/xxx/environ not an option? This way we would not have to save them specifically.

Restoring every env variable basically disables the ability of an external app to run with a different environment than the one AppRun had at startup

... if an exec wrapper is used. To be this would be an acceptable side effect for the (few) AppImages that use an exec wrapper, as long as it is clearly documented.

As I have said, I think we should do this directly in the upstream AppImageKit rep

For now, I would like to keep this separate (in this repo) so that people can explicitly opt into this when needed. As for "mainstream" AppImages, I have been experimenting (e.g., in linuxdeployqt) to do away with exported environment variables such as LD_LIBRARY_PATH altogether since I think this is a cleaner solution.

from linuxdeploy-plugin-checkrt.

Ok, got it.

Is retrieving them from /proc/xxx/environ not an option? This way we would not have to save them specifically.

I tried getting that before and after modifying the variables. It seems that doesn't hold the value it had at startup, but the value it has now. So, the moment we modify any env variable, we can't retrieve the original values anymore if we don't explicitly copy them.

from linuxdeploy-plugin-checkrt.

Can we get the env of the parent process? Might be good enough?

from linuxdeploy-plugin-checkrt.

No worries. Getting the startup env is not an issue.

1. We just need to copy its state when starting AppRun. It's basically copying http://man7.org/linux/man-pages/man7/environ.7.html.

2. Then, we inject exec.so through LD_PRELOAD

3. This exec.so will hijack the exec calls and will replace the environment with the startup environment in case the executable lives within APPDIR. However, there's no easy way for exec to get the original values copied at 1. How do we pass them? Through yet another env var that we parse? Or as you suggested we forget about 1 and just take the env of the parent process? This may be different from when the application started but is not a bad assumption.

Then, if you want to restore the startup variables for external processes, you copy exec.so into your APPDIR/optional folder. Otherwise, you just don't copy it.

Does it sound fair?

from linuxdeploy-plugin-checkrt.

Sounds good to me!

from linuxdeploy-plugin-checkrt.