Skip to content

librtmp: Fix reset socket descriptor to -1 after closing RTMP connect… #12207

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lilinxiong
Copy link

@lilinxiong lilinxiong commented May 26, 2025

Description

  1. The socket descriptor (sb_socket) is explicitly set to -1 after closing in the WriteN function.

  2. The client ID cleanup logic in RTMP_Close is moved after the socket closure to maintain proper resource cleanup order.

Motivation and Context

#03 pc 000000000045d7cc /lib/arm64/liblive-push.so (RTMPSockBuf_Close+112) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)
#04 pc 000000000045d56c /lib/arm64/liblive-push.so (WriteN+420) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)
#05 pc 000000000045c4cc /lib/arm64/liblive-push.so (RTMP_SendPacket+1964) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)
#06 pc 0000000000459e48 /lib/arm64/liblive-push.so (SendFCUnpublish+264) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)
#07 pc 0000000000456e2c /lib/arm64/liblive-push.so (RTMP_Close+196) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)
#08 pc 000000000045d574 /lib/arm64/liblive-push.so (WriteN+428) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)
#09 pc 000000000045c4cc /lib/arm64/liblive-push.so (RTMP_SendPacket+1964) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)
#10 pc 000000000045f0d0 /lib/arm64/liblive-push.so (RTMP_Write+744) (BuildId: 7847e45a2319a0441b147ccfbdda6841f007740f)

I was using RTMP for live streaming on Android, and when I shut down the server, the above stack trace appeared. After investigation, the cause was as follows:

  1. The immediate cause is that Android prohibits calling close() on a socket that has already been closed.
  2. In RTMP_Close(), if the current sb_socket is still valid, the flow re-enters WriteN() via SendFCUnpublish() -> RTMP_SendPacket(). This causes RTMPSockBuf_Send() to fail again, leading back to RTMP_Close() and creating an infinite loop.

How Has This Been Tested?

Types of changes

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@RytoEX RytoEX requested review from notr1ch and tt2468 May 27, 2025 18:06
@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label May 28, 2025
@norihiro
Copy link
Contributor

Please remove the punctuation (.) from the commit message title.
(You can edit it by git commit --amend and force push to GitHub.)

@lilinxiong lilinxiong force-pushed the fix/socket_close_crash branch from 44fd087 to 7ec6d2f Compare June 3, 2025 05:45
@lilinxiong
Copy link
Author

Please remove the punctuation (.) from the commit message title. (You can edit it by git commit --amend and force push to GitHub.)

done.

@notr1ch
Copy link
Member

notr1ch commented Jun 3, 2025

Wouldn't doing this in RTMPSockBuf_Close be a more appropriate place?

@lilinxiong lilinxiong force-pushed the fix/socket_close_crash branch from 7ec6d2f to cf22325 Compare June 4, 2025 05:36
@lilinxiong
Copy link
Author

Wouldn't doing this in RTMPSockBuf_Close be a more appropriate place?

You are right.

@Fenrirthviti
Copy link
Member

Out of curiosity, where exactly is that stack from? I'm unable to find any references to a liblive-push anywhere. Is this something based on librtmp?

@lilinxiong
Copy link
Author

Out of curiosity, where exactly is that stack from? I'm unable to find any references to a liblive-push anywhere. Is this something based on librtmp?

liblive-push is a library we use in our own project for live streaming, and yes, it is based on librtmp.

@Fenrirthviti
Copy link
Member

Is this something that you are distributing? Is the source available? I'm reluctant to accept a fix based on a project that might be violating the librtmp license without understanding the context a bit more. Thanks in advance.

@lilinxiong
Copy link
Author

Is this something that you are distributing? Is the source available? I'm reluctant to accept a fix based on a project that might be violating the librtmp license without understanding the context a bit more. Thanks in advance.

Since it’s my own demo project and it’s not fully developed yet, it will be available on GitHub later with a public repository.
If it’s absolutely necessary to wait until my demo project is fully open-sourced before merging, it will likely take 1 to 3 weeks, as I can only work on it during my spare time. However, this bug is a logical error.

@Fenrirthviti
Copy link
Member

That should be fine then, no need to delay for your release if it is just internal for the moment. Thanks for the context, we were just curious about the crash stack and couldn't find the library anywhere.

@lilinxiong
Copy link
Author

That should be fine then, no need to delay for your release if it is just internal for the moment. Thanks for the context, we were just curious about the crash stack and couldn't find the library anywhere.

So can it be merged normally now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants