opensciencegrid / xrootd-multiuser

A filesystem plugin to allow Xrootd write as a different Unix user
Apache License 2.0
2 stars 12 forks source link

Avoid segfault for anonymous clients with macaroons #39

Closed jthiltges closed 7 months ago

jthiltges commented 1 year ago

Don't dereference a null sentryPtr Closes #38

djw8605 commented 1 year ago

If the username is "", would it always hit this line:

https://github.com/opensciencegrid/xrootd-multiuser/blob/642a8f2843b27e5c76d7d872cf75801e68be72a1/src/UserSentry.hh#L125-L131

jthiltges commented 1 year ago

Thanks for pointing out that mistake. I reworked things a little. I do like having anonymous access logged differently from lookup failures (and avoiding the overhead).

Note that I still need to compile and test this (the reason for marking it draft).

djw8605 commented 1 year ago

Let me know when you have had a chance to compile and test.

jthiltges commented 1 year ago

@djw8605 I've built xrootd-multiuser with this patch, and it's running on red-xfer13.unl.edu. Initial testing looks OK. Anonymous and authenticated requests seem OK.

matyasselmeci commented 10 months ago

Is this still an issue?

jthiltges commented 10 months ago

It's still reproducible with the example in #38

[root@red-xfer13 ~]# rpm -q xrootd xrootd-multiuser
xrootd-5.6.2-2.3.osg36.el8.x86_64
xrootd-multiuser-2.1.3-1.3.osg36.el8.x86_64

[root@red-xfer13 ~]# tail -0f /var/log/messages
Nov 13 17:19:36 red-xfer13.unl.edu kernel: xrootd[1622077]: segfault at 4 ip 00007f83cc9f0585 sp 00007f83d34e3fc0 error 4 in libXrdMultiuser-5.so[7f83cc9e3000+1a000]
Nov 13 17:19:36 red-xfer13.unl.edu kernel: Code: ff 0f 85 59 ff ff ff e9 59 ff ff ff f3 0f 1e fa eb 92 f3 0f 1e fa 49 89 c4 e9 d4 fe ff ff 48 c7 84 24 90 00 00 00 00 00 00 00 <8b> 04 25 04 00 00 00 0f 0b f3 0f 1e fa 49 89 c4 e9 bb fe ff ff 66
Nov 13 17:19:36 red-xfer13.unl.edu systemd[1]: xrootd-privileged@clustered.service: Main process exited, code=killed, status=11/SEGV
Nov 13 17:19:36 red-xfer13.unl.edu systemd[1]: xrootd-privileged@clustered.service: Failed with result 'signal'.
matyasselmeci commented 10 months ago

Derek, do you think you can take a look in the next few days? I was about to cut a release so we can make #53 available, but I can hold off on tagging until this is merged...

bbockelm commented 8 months ago

@djw8605 - ping on this item.

bbockelm commented 7 months ago

@djw8605 - Ping on the above!