Skip to content

LuaSocket: Bump to 20240524#1827

Merged
NiLuJe merged 1 commit into
koreader:masterfrom
NiLuJe:master
Jun 18, 2024
Merged

LuaSocket: Bump to 20240524#1827
NiLuJe merged 1 commit into
koreader:masterfrom
NiLuJe:master

Conversation

@NiLuJe

@NiLuJe NiLuJe commented Jun 17, 2024

Copy link
Copy Markdown
Member

And enforce CLOEXEC on open'ed sockets, to avoid weird crap like koreader/koreader#12043


This change is Reviewable

And enforce CLOEXEC on open'ed sockets, to avoid weird crap like koreader/koreader#12043
@Frenzie

Frenzie commented Jun 17, 2024

Copy link
Copy Markdown
Member

Is it suitable to PR upstream as is?

@NiLuJe

NiLuJe commented Jun 17, 2024

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

NiLuJe commented Jun 17, 2024

Copy link
Copy Markdown
Member Author

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

NiLuJe commented Jun 17, 2024

Copy link
Copy Markdown
Member Author

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 ;).

Comment thread thirdparty/luasocket/CMakeLists.txt
@NiLuJe NiLuJe merged commit 7b14647 into koreader:master Jun 18, 2024
@pazos

pazos commented Jun 30, 2024

Copy link
Copy Markdown
Member

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

@NiLuJe

NiLuJe commented Jul 1, 2024

Copy link
Copy Markdown
Member Author

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

In case bionic did a bionic...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants