Page MenuHomePhabricator

Add support for MIT-SHM FD-passing
ClosedPublic

Authored by avolkov on Feb 2 2018, 2:55 AM.

Details

Reviewers
kwo
Summary

This is more secure way of using shared memory because
it's visible only to the X server and the application.

Diff Detail

Branch
ximage-shm
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5725
Build 6361: arc lint + arc unit
avolkov requested review of this revision.Feb 2 2018, 2:55 AM
avolkov created this revision.

It's more convenient to view diff with "git show HEAD -w"

kwo added a comment.Feb 3 2018, 9:05 AM

Looks good to me otherwise, and even seems to work :)

configure.ac
463

When running configure it looks like this option is in the "Image Loaders" department.
I think a better place would be e.g. "Use X MIT-SHM FD-passing" below "Use visibility hiding"

src/lib/Makefile.am
90–91

I suggest to make this all
MY_LIBS += -lXext -lX11 $(X_SHM_FD_LIBS)

src/lib/ximage.c
92

We don't use const variables like this anywhere else in imlib2 so I'd prefer not starting now.

96–97

I think we need

si->shmaddr = NULL;

around here or I fear that things could go horribly wrong.

105–109

Pointless cast in C land.

200

x_does_shm_fd?

avolkov updated this revision to Diff 13806.Feb 5 2018, 4:57 AM

Rebased

kwo added inline comments.Feb 5 2018, 7:47 AM
configure.ac
124

Apparently this should go outside the if (e.g. below AM_CONDITIONAL(BUILD_X11...), otherwise the build fails --without-x.
Also, in the else case below x_shm_fd="no" should be added or it will be blank in the summary if configuring --without-x.

src/lib/ximage.c
127–132

Hmm... I think line 127 to 130 should be

x_does_shm_fd = 0;

and the regular shm case should not be in an else clause.

This way we would try to fall back to regular shm, but maybe this was intentional?

avolkov added inline comments.Feb 5 2018, 8:22 AM
src/lib/ximage.c
127–132

Yes, it's intentional.
If there is a secure way to use shm, then use only it.
If it fails, then it fails likely because there is not enough memory and the regular shm will likely fail too.

avolkov updated this revision to Diff 13807.Feb 5 2018, 8:35 AM

Fix build with --without-x

kwo added a comment.Feb 5 2018, 10:04 AM

No fallback to regular - sounds reasonable.

configure.ac
128

You forgot to re-add AM_CONDITIONAL(HAVE_X11_SHM_FD... here?

kwo accepted this revision.Feb 5 2018, 9:26 PM

Looks good to me.

Will push soonish.

Thanks :)

configure.ac
128

Forget it. I see it's no longer needed.

This revision is now accepted and ready to land.Feb 5 2018, 9:26 PM
kwo closed this revision.Sep 30 2019, 10:30 AM