SUSE / libpulp

libpulp enables live patching in user space applications.
GNU Lesser General Public License v2.1
55 stars 11 forks source link

Implicit reliance on '/' to verify libpulp.so loaded. Needs documentation #26

Closed a-gavin closed 3 years ago

a-gavin commented 3 years ago

When attempting to run the example from libpulp(7) using a relative path to libpulp.so (e.g. LD_PRELOAD=../lib/.libs/libpulp.so ./program), ulp_trigger exits with exit code 1 and prints the following:

ulp: libpulp not loaded, thus live patching not possible.
ulp: unable to get in-memory information about shared libraries.
ulp: error gathering target process information.

On this line in tools/introspection.c, ulp_trigger implicitly checks to see if the currently-considered dynamically loaded object is not from the default search path. Libraries that are on the default search path are simply named <library_name>.so, that I am aware.

The source of the issue is libname read from the symbol table for libpulp is ../lib/.libs/libpulp.so when run with a relative path. The program instead expects /<full_path_to>/libpulp.so. This causes the implicit check to fail.

Running the test program as below fixed the issue:

I can understand why this check exists in its current form. The use of a full path to libpulp could be better documented in libpulp(7), though. Will submit a PR for this.

inconstante commented 3 years ago

Hello,

I'd like to let you know that you are not the first person to hit that! I did it and others at SUSE did it, too. So, I'm sorry that this is still happening. :|

I'd also like to suggest removing the check (libname[0] == '/') altogether.

a-gavin commented 3 years ago

Hello,

I'd like to let you know that you are not the first person to hit that! I did it and others at SUSE did it, too. So, I'm sorry that this is still happening. :|

Everything has a learning curve, I suppose!

I'd also like to suggest removing the check (libname[0] == '/') altogether.

Happy to make this change, as you mentioned in #28. Will have PR up after I verify behavior will be as expected w/ this removal

a-gavin commented 3 years ago

Submitted PR which fixes issue, save one caveat.

Other than a user-specified DSO, as mentioned in my original post, the only non-full path DSO loaded in my basic tests was linux-vdso.so.1 on my system. See vdso(7) for more details.

By removing the libname check, the vdso DSO is erroneously added to the target libraries here after it fails previous conditionals. This will then be visible to the end user as a patchable library. I would have to do some additional investigation to properly remove this.

This is the only issue I am aware of having traced the execution of ulp_trigger. Although, given my relative lack of experience with the package, it is possible I have missed other potential references to vDSO as a target library.

inconstante commented 3 years ago

By removing the libname check, the vdso DSO is erroneously added to the target libraries here after it fails previous conditionals. This will then be visible to the end user as a patchable library. I would have to do some additional investigation to properly remove this.

That's a good point, but a little bit of history will make you worry less...

In the past, Libpulp tracked the application-library boundary on a thread-by-thread basis. This tracking was not performed by libpulp.so, but rather by the target library itself. On top of that, every wannabe-live-patchable library had to expose the results of the tracking to the live patching infrastructure, and they did so with the help of a function (__ulp_get_local_universe). Finally, Libpulp's tools relied on the presence or absence of this function to determine whether a target library was live patchable or not.

So, before commit f85145013ca2, we used to distinguish between live-patchable and non-live-patchable libraries, but after that commit, the distinction is gone (see the highlights in the following link and notice how dynobj_targets and dynobj_others became a single variable).

Adding another non-live-patchable library (even if it's the vdso) to the list of libraries in a process is not a regression; other non-live-patchable libraries were already part of that list.

inconstante commented 3 years ago

Fixed by 2e790a15612c.