bitwiseworks / libc

LIBC Next (kLIBC fork)
9 stars 4 forks source link

_impsockhandle imports socket as new #95

Open dmik opened 3 years ago

dmik commented 3 years ago

From https://github.com/bitwiseworks/libcx/issues/87#issuecomment-772858865:

Okay, I sorted that out: _impsockhandle imports the socket as new to LIBC which results in addsockettolist not being called and also causes close in the other process to call soclose (which effectively and globally shuts down the socket) rather than just decrease the socket reference counter for that process. So it's not a surprise a subsequent read fails with EOF. It's a surprise that it works then if DosSleep is used... Anyway, this looks like a LIBC bug to me. Or, at least, bug-o-feature.

Using TCPNAMEG(AllocFHEx) directly to import the socket as "old" rather than as "new" fixes the problem completely. I will create a LIBC ticket for the record.

It's not clear what rationale stands behind that — perhaps, it was assumed that the imported socket is tracked where it comes from else and hence should not be addsockettolisted in this process or such. But this is surely wrong in case when the socket comes from another LIBC process (where it is already tracked). Besides breaking close/read functionality this also leaves the socket "orphan" due to `addsockettolist not being called...

I guess the best solution here (to maintain a possible backward compatibility) is to use a (currently unused) flags parameter of _impsockhandle to specify if the socket should be imported as new or old.

I need an opinion from the author to be sure (or another case where it matters). Postponed for now as can be easily worked around with TCPNAMEG(AllocFHEx) in LIBCx (where it is hidden from the API user anyway).