Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

zdohnal
Copy link
Contributor

@zdohnal zdohnal commented Feb 9, 2024

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

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 michaelrsweet#331
Copy link
Owner

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

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?

michaelrsweet added a commit that referenced this pull request Feb 9, 2024
michaelrsweet added a commit that referenced this pull request Feb 9, 2024
@michaelrsweet
Copy link
Owner

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

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

@michaelrsweet michaelrsweet self-assigned this Feb 9, 2024
@michaelrsweet michaelrsweet added bug Something isn't working priority-low labels Feb 9, 2024
@michaelrsweet michaelrsweet added this to the Stable milestone Feb 9, 2024
@zdohnal
Copy link
Contributor Author

zdohnal commented Feb 12, 2024

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.

michaelrsweet added a commit that referenced this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raw socket gets data, but does not send data to printer
2 participants