Cleanup .gnu.version_r after removing symbol version

[sulix]

If --clear-symbol-version is used to remove all occurrances of a symbol version, it nevertheless remains in the .gnu.version_r section as being required by a particular dependency.

Instead, after removing symbol versions, loop over the .gnu.version and gnu.version_r tables to find any unused version, and unlink them from the linked-lists in the .gnu.version_r section.

While an ideal implementation would probably rebuild the entire .gnu.version_r section, this works well enough to stop objdump noting the no-longer-valid version dependency.

Note that the test is very glibc-specific — it works on my machine, but I wouldn't trust it too much further. I assume that's one of the reasons symbol versions aren't tested much yet, so if you'd rather I remove the test, that's fine.

[martinetd]

Thank you! This is awesome! You're doing pretty much exactly what I was suggesting, so I'm obviously happy with it :)
I've tested it on the couple of binaries I need to edit and it works as advertised for me.

For the test, I agree it's not great as is, and checking we don't remove too much in .gnu.version_r by adding two symbols and removing them one at a time?
I've just tried that, but I managed to produce a binary with versions without gnu.version_r and patchelf fails on these:

$ cat foo.map 
VERS1 {
  global:
    vers1_1; vers1_2;
  local:
    *;
};

VERS2 {
  global:
    vers2;
  local:
    *;
} VERS1;
$ cat foo.c
void vers1_1(void) {}
void vers1_2(void) {}
void vers2(void) {}
$ gcc -Wl,--version-script=foo.map -fpic -shared -o libfoo.so foo.c
$ ~/code/patchelf/src/patchelf --clear-symbol-version vers1_1 libfoo.so
patchelf: cannot find section '.gnu.version_r'
$ echo $?
1
$ readelf -V libfoo.so 

Version symbols section '.gnu.version' contains 10 entries:
 Addr: 0x000000000000040a  Offset: 0x00040a  Link: 3 (.dynsym)
  000:   0 (*local*)       1 (*global*)      1 (*global*)      1 (*global*)   
  004:   1 (*global*)      2 (VERS1)         2 (VERS1)         3 (VERS2)      
  008:   3 (VERS2)         2 (VERS1)      

Version definition section '.gnu.version_d' contains 3 entries:
 Addr: 0x0000000000000420  Offset: 0x000420  Link: 4 (.dynstr)
  000000: Rev: 1  Flags: BASE  Index: 1  Cnt: 1  Name: libfoo.so
  0x001c: Rev: 1  Flags: none  Index: 2  Cnt: 1  Name: VERS1
  0x0038: Rev: 1  Flags: none  Index: 3  Cnt: 2  Name: VERS2
  0x0054: Parent 1: VERS1

I'm running out of time right now but I'll have a look tomorrow to see if I can get it to use version_r instead for our tests.

[Mic92]

Sorry for the late review, but never now than never...
I think this test should not fail on non-glibc machines as we also have openbsd/freebsd user:

Suggested change

	exit 1
	exit 0

[sulix]

Good idea. I've changed this, and improved the message printed.

[Mic92]

Can you still make the test not fail in this case? It also effects our musl builds, that we use for static binaries.

[sulix]

Whoops — I didn't notice that git was stuck asking for the password to push this. I've actually uploaded the new version with that changed. Sorry!

[Mic92]

Does this not break when we upgrade glibc or do I miss something?

[sulix]

Yes, but rarely.

glibc only updates symbol versions if that particular function has changed in an incompatible way. For the __libc_start_main symbol, it has changed exactly once, to introduce the GLIBC_2.34 version. It's not particularly likely that this function will change in an incompatible way regularly, so it seemed a reasonably safe bet that this'll still work for the next 5–10 years.

[Mic92]

I'm running out of time right now but I'll have a look tomorrow to see if I can get it to use version_r instead for our tests.

Do you still plan to do this?

[martinetd]

I'm running out of time right now but I'll have a look tomorrow to see if I can get it to use version_r instead for our tests.

Do you still plan to do this?

Sorry didn't manage to do it quickly enough back then and forgot about it...
Turns out I was silly and too focused on producing a lib with multiple versions (.gnu.version_d), but version_r is about using anything we care about so it's much simpler:

$ cat user.c
#include <sys/stat.h>

int foo() {
	struct stat buf;

	fstat(0, &buf);
	stat(".", &buf);
	return 0;
}
$ gcc -fpic -shared -o libuser.so user.c
$ nm -D libuser.so | grep @
                 w __cxa_finalize@GLIBC_2.2.5
                 U fstat@GLIBC_2.33
                 U stat@GLIBC_2.33
$ objdump -x libuser.so | grep -A 3 Version
Version References:
  required from libc.so.6:
    0x09691a75 0x00 03 GLIBC_2.2.5
    0x069691b3 0x00 02 GLIBC_2.33
$ ../src/patchelf --clear-symbol-version stat libuser.so
$ nm -D libuser.so | grep @
                 w __cxa_finalize@GLIBC_2.2.5
                 U fstat@GLIBC_2.33
$ objdump -x libuser.so | grep -A 3 Version
Version References:
  required from libc.so.6:
    0x09691a75 0x00 03 GLIBC_2.2.5
    0x069691b3 0x00 02 GLIBC_2.33
$ ../src/patchelf --clear-symbol-version fstat libuser.so
$ nm -D libuser.so | grep @
                 w __cxa_finalize@GLIBC_2.2.5
$ objdump -x libuser.so | grep -A 3 Version
Version References:
  required from libc.so.6:
    0x09691a75 0x00 03 GLIBC_2.2.5

( so 2.33 is gone after removing both symbols and only after removing both symbols from it - which is what I think I wanted to test?)

ideally could go as far as actually running it to make sure linker is happy, but that'd mean providing the symbols and that feels too much like work to do on a busy weekend; please tell me and I'll try not to forget this time :)

[sulix]

I needed this again today, so I rebased the change, and tidied up the test slightly.

It's still not the ideal way of testing this: we probably should build our own separate library with symbol versions, and a program which links against it. But that's significantly more work, and glibc's own symbol versions are pretty much everyone's use-case.

[Mic92]

Hi. I opened up a new PR to add some more pointer checks on top. I know this is not consequently done in our codebase yet, but I want to converge over time to it: e1aa6d6

[bobbobbio]

I found myself wanting this today. Is there any chance this will eventually get merged in the future? What are the outstanding concerns with this approach? I couldn't understand from the existing comments.