lincheney / fzf-tab-completion

Tab completion using fzf
GNU General Public License v3.0
623 stars 40 forks source link

Is the readline implementation correct? #75

Open psacawa opened 1 year ago

psacawa commented 1 year ago

Hello, thank you for your efforts in the direction of fzf+readline integration. However, I have to wonder if the current suggested integration strategy actually works. It suggests the .inputrc config

$include function rl_custom_complete /path/to/fzf-tab-completion/readline/target/release/librl_custom_complete.so

I had no idea that libreadline can dynamically load functions from any shared object. However, looking at the documentation, I don't see any mention of that config syntax. For them $include is just something like #include. Additionally, libreadline doesn't require use symbol dlopen on my system, nor does any commit in the upstream repository contain "$include function".

So are you using a custom fork of libreadline to make this work? I am not able to get it to work. I think I'll statically link your project together with my own libreadline.

Edit: Indeed, tracing sqlite3 with the example config I find it tries to open the file whose name is function rl_custom_complete /home/psacawa/lib/librl_custom_complete.so. Where did you get libreadline.so that works this way?

Edit: Fixed, see below

psacawa commented 1 year ago

I have reread the documentation and seen https://github.com/lincheney/rl_custom_function . Homewever, there is another concern. The application works by hijacking rl_parse_and_bind in the runtime linker. However, when I test with e.g. sqlite, it does not work. When I check the call stacks the get to rl_parse_and_bind in gdb, I find they look as follows:

[0] from 0x00007ffff7e193b0 in rl_parse_and_bind
[1] from 0x00007ffff7e19e75
[2] from 0x00007ffff7e19fe0 in rl_read_init_file
[3] from 0x00007ffff7e13c6e in rl_initialize
[4] from 0x00007ffff7e13eff in readline
[5] from 0x0000555555583682
[6] from 0x000055555555e8fd
[7] from 0x00007ffff7a23510 in __libc_start_call_main+128 at ../sysdeps/nptl/libc_start_call_main.h:58
[8] from 0x00007ffff7a235c9 in __libc_start_main_impl+137 at ../csu/libc-start.c:381
[9] from 0x000055555555f5e5

The call to rl_parse_and_bind was local to libreadline, and did not use the PLT, so was not intercepted the linker. On my system, the stack frame above rl_parse_and_bind [1] is a location in libreadline.so .text section past the end of the rl_parse_and_bind symbol, but not attached to any symbol. I suppose this means it was attached to a stripped symbol. In any case, it cannot be intercepted unless libreadline is built with some special flags to make the function call go through the PLT.

lincheney commented 1 year ago

Hello

It works for me ... but that is probably not a helpful response :|

I suspect this issue may be related: https://github.com/lincheney/rl_custom_function/issues/5 Like you have noted, it cannot be intercepted unless libreadline is built with some special flags; I suspect this is the crux of it, your libreadline is built differently from mine in an important way, that means it doesn't work properly.

It works well for the official readline packages from the 2 linux distros I use (fedora + arch), but that is hardly comprehensive. I would be interested to know what you are using / how you have installed libreadline.

(Also, if you have suggestions on better ways to implement this, please let me know!)

psacawa commented 1 year ago

I am using libreadline 8.2 disstributed from the package manager on Ubuntu 22.10. I've tried to elicit a function call to use the PLT in a function call that does not cross shared library boundary using various linker options, but couldn't figure it out.

This is different from various statements I've read online, such as this answer: "With -fPIC instead of -fPIE, things are even worse: even calls to global functions defined within the same compilation unit have to go through the PLT, to support symbol interposition." This is not true in my experience. It was not an issue of default linker arguments (things like -z now -z relro). I crafted the ld invocation manually.

I also tried to look at readline packages on fedora package index. For an old one, I can see there was such a function call:

> objdump -d readline-7.0-11.fc29/usr/lib64/libreadline.so.7.0 | grep rl_parse_and_bind@plt
0000000000015470 <rl_parse_and_bind@plt>:
   247cd:       e8 9e 0c ff ff          call   15470 <rl_parse_and_bind@plt>

However, for a newer one, that call was no longer there. I don't think you can count on this rl_parse_and_bind@plt call in present+future versions of fedora. Which version are you using?

I would guess that for performance reasons, PLT calls are being phased out by linkers when possible. This means that you can only hijack calls between shared objects. In the case of readline, the only one you can be sure of is the call to char* readline(const char*) itself. readline has these interfaces with extern variables which you can't intercept. I thing you must intercept readline only. Following pseudocode might help:

#include <dlfcn.h>

char *(*original_readline)(const char *) = 0;
int fzf_readline_initialised = 0;

void __attribute__((constructor)) readline_proxy_init() {
  original_readline = dlsym(RTLD_NEXT, "readline");
}

void fzf_readline_initialise() {
  /* initialization that can't work in constructor */
  /* ... */
  fzf_readline_initialised = 1;
}

char *readline(const char *prompt) {
  if (!fzf_readline_initialised) {
    fzf_readline_initialise();
  }
  return (*original_readline)(prompt);
}

However, I don't know the readline internals.

lincheney commented 1 year ago

Hi

I'm using Fedora 37 right now with the latest available readline. Interestingly, I downloaded the ones for 38 and 39 (rawhide) and they seemed to work fine as well. However, I got the Ubuntu 22 readline and that indeed did not work.

In the case of readline, the only one you can be sure of is the call to char readline(const char) itself

Yeah this is probably true. The tricky bit will be how to configure the extra readline functions as it will no longer be able to do things with the inputrc. A long time ago, this used to be specified with environment variables, I may have to revert to doing that. I did a simple test to override readline() and make it print a hello world, which worked ... except on python3. Somehow python3 is doing something differently; I will have to investigate further.

psacawa commented 1 year ago

It looks there is an alternate interface which readline module uses.

srd424 commented 12 months ago

I threw together a horrible hack (in an ancient language, because I'm a dinosaur):

https://gist.github.com/srd424/e8649dae5cd6c3b17ac0d7de0ffcb2ff

Compared to @lincheney's implementation it rather demonstrates why I shouldn't be allowed near a compiler, but it does seem to work for the moment :)