search

michaelrsweet / pappl

PAPPL - Printer Application Framework
https://www.msweet.org/pappl
Apache License 2.0
309 stars 49 forks source link

Fix netcat hang when it send raw data to raw socket #338

Closed zdohnal closed 8 months ago

zdohnal commented 8 months ago

netcat and similar tools hang when they send data to PAPPL's raw socket. It is because these tools wait for input from the destination, and we never end because update activity every time poll() returns non-negative value, and the tools never send POLLHUP.

The proposed fix is to update activity only when we read from the socket, and break out of the loop after 10s of inactivity.

Fixes #331

michaelrsweet commented 8 months ago

[master a483c72] Track activity only on read (Issue #338)

[v1.4.x 06f8942] Track activity only on read (Issue #338)

zdohnal commented 8 months ago

netcat doesn't behave like the socket backend or any other JetDirect client - when you are done sending data your data you either close the socket or shutdown the outgoing part of the socket, which will signal PAPPL that things are done. netcat is more like a dumb implementation of telnet...

I checked netcat and nc (nmap implementation of netcat iiuc) and both were polling the descriptor for events when the binary looked frozen, and the printer application was in pappl iterating in infinite loop with poll(). I'll check the fix in the latest release and let you know if it helps.

Also, POLLHUP is output only; from the man page:

     POLLHUP        The device or socket has been disconnected.  This flag is
                    output only, and ignored if present in the input events
                    bitmask.  Note that POLLHUP and POLLOUT are mutually
                    exclusive and should never be present in the revents
                    bitmask at the same time.

I've added it to requested events only for formal reasons - IIUC 'man poll', POLLHUP is always returned in revents if appeared, I meant it like stressing out we check POLLHUP event later (even if it is ignored in events).

As for only updating activity when there is data to be read, it makes sense so that part of your change at least can be adopted.

Finally, it looks like you have a bunch of gratuitous whitespace changes?

Bunch of blocks did not follow the code style (unless you've changed the guidelines to differ from CUPS) in the function - 8 spaces weren't substituted by a tab - so I've updated the code to follow the rule. I made it as a separate commit in the PR, so anyone backporting the real fix won't get unnecessary whitespace changes.