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

HTTP-FLV: Crash when multiple viewers. v6.0.148 v7.0.5 #4144

Merged

Conversation

retamia
Copy link
Contributor

@retamia retamia commented Aug 14, 2024

I did some preliminary code inspection. The two playback endpoints share the same SrsLiveStream instance. After the first one disconnects, alive_ is set to false.

  alive_ = true;
  err = do_serve_http(w, r);
  alive_ = false;

In the SrsHttpStreamServer::http_unmount(SrsRequest* r) function, stream->alive() is already false, so mux.unhandle will free the SrsLiveStream. This causes the other connection coroutine to return to its execution environment after the SrsLiveStream instance has already been freed.

    // Wait for cache and stream to stop.
    int i = 0;
    for (; i < 1024; i++) {
        if (!cache->alive() && !stream->alive()) {
            break;
        }
        srs_usleep(100 * SRS_UTIME_MILLISECONDS);
    }

    // Unmount the HTTP handler, which will free the entry. Note that we must free it after cache and
    // stream stopped for it uses it.
    mux.unhandle(entry->mount, stream.get());

alive_ was changed from a bool to an int to ensure that mux.unhandle is only executed after each connection's serve_http has exited.

@winlinvip winlinvip force-pushed the bugfix/unpublish-crash-when-multi-viewers branch from 621243a to c3b6d97 Compare August 15, 2024 03:16
@winlinvip winlinvip changed the title Bugfix/unpublish crash when multi viewers HTTP-FLV: Crash when multiple viewers Aug 15, 2024
@winlinvip winlinvip merged commit fc9ca39 into ossrs:develop Aug 15, 2024
17 checks passed
@winlinvip winlinvip changed the title HTTP-FLV: Crash when multiple viewers HTTP-FLV: Crash when multiple viewers. v6.0.148 v7.0.5 Aug 15, 2024
winlinvip added a commit that referenced this pull request Aug 15, 2024
I did some preliminary code inspection. The two playback endpoints share
the same `SrsLiveStream` instance. After the first one disconnects,
`alive_` is set to false.
```
  alive_ = true;
  err = do_serve_http(w, r);
  alive_ = false;
```

In the `SrsHttpStreamServer::http_unmount(SrsRequest* r)` function,
`stream->alive()` is already false, so `mux.unhandle` will free the
`SrsLiveStream`. This causes the other connection coroutine to return to
its execution environment after the `SrsLiveStream` instance has already
been freed.
```
    // Wait for cache and stream to stop.
    int i = 0;
    for (; i < 1024; i++) {
        if (!cache->alive() && !stream->alive()) {
            break;
        }
        srs_usleep(100 * SRS_UTIME_MILLISECONDS);
    }

    // Unmount the HTTP handler, which will free the entry. Note that we must free it after cache and
    // stream stopped for it uses it.
    mux.unhandle(entry->mount, stream.get());
```

`alive_` was changed from a `bool` to an `int` to ensure that
`mux.unhandle` is only executed after each connection's `serve_http` has
exited.

---------

Co-authored-by: liumengte <liumengte@visionular.com>
Co-authored-by: winlin <winlinvip@gmail.com>
winlinvip added a commit that referenced this pull request Aug 15, 2024
I did some preliminary code inspection. The two playback endpoints share
the same `SrsLiveStream` instance. After the first one disconnects,
`alive_` is set to false.
```
  alive_ = true;
  err = do_serve_http(w, r);
  alive_ = false;
```

In the `SrsHttpStreamServer::http_unmount(SrsRequest* r)` function,
`stream->alive()` is already false, so `mux.unhandle` will free the
`SrsLiveStream`. This causes the other connection coroutine to return to
its execution environment after the `SrsLiveStream` instance has already
been freed.
```
    // Wait for cache and stream to stop.
    int i = 0;
    for (; i < 1024; i++) {
        if (!cache->alive() && !stream->alive()) {
            break;
        }
        srs_usleep(100 * SRS_UTIME_MILLISECONDS);
    }

    // Unmount the HTTP handler, which will free the entry. Note that we must free it after cache and
    // stream stopped for it uses it.
    mux.unhandle(entry->mount, stream.get());
```

`alive_` was changed from a `bool` to an `int` to ensure that
`mux.unhandle` is only executed after each connection's `serve_http` has
exited.

---------

Co-authored-by: liumengte <liumengte@visionular.com>
Co-authored-by: winlin <winlinvip@gmail.com>
winlinvip added a commit that referenced this pull request Aug 31, 2024
…0.11 (#4164)

When stopping the stream, it will wait for the HTTP Streaming to exit.
If the HTTP Streaming goroutine hangs, it will not exit automatically.

```cpp
void SrsHttpStreamServer::http_unmount(SrsRequest* r)
{
    SrsUniquePtr<SrsLiveStream> stream(entry->stream);
    if (stream->entry) stream->entry->enabled = false;
    srs_usleep(...); // Wait for about 120s.
    mux.unhandle(entry->mount, stream.get()); // Free stream.
}

srs_error_t SrsLiveStream::serve_http(ISrsHttpResponseWriter* w, ISrsHttpMessage* r)
{
    err = do_serve_http(w, r); // If stuck in here for 120s+
    alive_viewers_--; // Crash at here, because stream has been deleted.
```

We should notify http stream connection to interrupt(expire):

```cpp
void SrsHttpStreamServer::http_unmount(SrsRequest* r)
{
    SrsUniquePtr<SrsLiveStream> stream(entry->stream);
    if (stream->entry) stream->entry->enabled = false;
    stream->expire(); // Notify http stream to interrupt.
```

Note that we should notify all viewers pulling stream from this http
stream.

Note that we have tried to fix this issue, but only try to wait for all
viewers to quit, without interrupting the viewers, see
#4144


---------

Co-authored-by: Jacob Su <suzp1984@gmail.com>
winlinvip added a commit that referenced this pull request Aug 31, 2024
)

When stopping the stream, it will wait for the HTTP Streaming to exit.
If the HTTP Streaming goroutine hangs, it will not exit automatically.

```cpp
void SrsHttpStreamServer::http_unmount(SrsRequest* r)
{
    SrsUniquePtr<SrsLiveStream> stream(entry->stream);
    if (stream->entry) stream->entry->enabled = false;
    srs_usleep(...); // Wait for about 120s.
    mux.unhandle(entry->mount, stream.get()); // Free stream.
}

srs_error_t SrsLiveStream::serve_http(ISrsHttpResponseWriter* w, ISrsHttpMessage* r)
{
    err = do_serve_http(w, r); // If stuck in here for 120s+
    alive_viewers_--; // Crash at here, because stream has been deleted.
```

We should notify http stream connection to interrupt(expire):

```cpp
void SrsHttpStreamServer::http_unmount(SrsRequest* r)
{
    SrsUniquePtr<SrsLiveStream> stream(entry->stream);
    if (stream->entry) stream->entry->enabled = false;
    stream->expire(); // Notify http stream to interrupt.
```

Note that we should notify all viewers pulling stream from this http
stream.

Note that we have tried to fix this issue, but only try to wait for all
viewers to quit, without interrupting the viewers, see
#4144

---------

Co-authored-by: Jacob Su <suzp1984@gmail.com>
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.

heap-use-after-free crash occurs after the streaming endpoint disconnects.
2 participants