search

koreader / koreader-base

Base framework offering a Lua scriptable environment for creating document readers
http://koreader.rocks/
GNU Affero General Public License v3.0
130 stars 105 forks source link

LuaSocket: Bump to 20240524 #1827

Closed NiLuJe closed 2 months ago

NiLuJe commented 2 months ago

And enforce CLOEXEC on open'ed sockets, to avoid weird crap like https://github.com/koreader/koreader/issues/12043

This change is Reviewable

Frenzie commented 2 months ago

Is it suitable to PR upstream as is?

NiLuJe commented 2 months ago

That's a fair question. I have no idea if there's a Windows equivalent, and/or if it's something upstream would even want. (I imagine there are use-cases where you want your child processes to inherit sockets).

benoit-pierre commented 2 months ago

Wouldn't using SOCK_CLOEXEC make more sense?

SOCK_CLOEXEC Set the close-on-exec (FD_CLOEXEC) flag on the new file descriptor. See the description of the O_CLOEXEC flag in open(2) for reasons why this may be useful.

NiLuJe commented 2 months ago

It would, but it might not be supported by the crappy old kernels found on our oldest devices (and it might actually make the socket call fail instead of silently ignoring the unknown flag, I can't recall).

So, using fcntl is, unfortunately, a portability necessity.

NiLuJe commented 2 months ago

If I check my notes from the last time I looked into it, as far as Kobo is concerned, it would be safe, the only CLOEXEC compat issue I'm aware of is the lack of support for accept4; open/socket/inotify are OK.

I know that open is NOT okay on legacy Kindle/PB, though, so I'm assuming that's also true of socket there ;).

pazos commented 2 months ago

I would like to revert this on android. See the linked issue. I will check tomorrow that reverting does fix the issue.

NiLuJe commented 2 months ago

Could be interesting to try to just skip the CLOEXEC patch on Android?

In case bionic did a bionic...