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 panic for MP4 stream with audio after v1.3 #338

Closed
yousong opened this issue Apr 7, 2023 · 8 comments
Closed

Fix panic for MP4 stream with audio after v1.3 #338

yousong opened this issue Apr 7, 2023 · 8 comments
Labels
bug Something isn't working
Milestone

Comments

@yousong
Copy link
Contributor

yousong commented Apr 7, 2023

I am wondering maybe it's caused by http.ResponseWriter being written concurrently from multiple goroutine.

The source is a ffmpeg:rtsp://.../stream1 with h264 video and pcma audio track.

  • The issue happens when accessing go2rtc /api/stream.mp4?src=tl_ipc44gw_main&video=h264,h265&audio=aac,opus,mp3,pcma,pcmu
  • No issue when accessing /api/stream.mp4?src=tl_ipc44gw_main. It seems in this case go2rtc defaults to audio=aac and the resulting mp4 stream is has only a video track present.

go2rtc runs as a armv7 container image (alexxit/go2rtc:master, alexxit/go2rtc@sha256:5e890220e10a482bf25032a99d86a3ab441a7def3a7d218aec92f1e0f57f8333).

The issue exhibits itself in two ways in the log.

  1. bufio slice bounts out of range.
panic: runtime error: slice bounds out of range [:2964] with capacity 2048

goroutine 519 [running]:
bufio.(*Writer).Flush(0x2572700)
        bufio/bufio.go:628 +0x1ac
bufio.(*Writer).Write(0x2572700, {0x3884800, 0x46c, 0x800})
        bufio/bufio.go:672 +0xdc
net/http.(*response).write(0x106a280, 0x46c, {0x3884800, 0x46c, 0x800}, {0x0, 0x0})
        net/http/server.go:1636 +0x214
net/http.(*response).Write(0x106a280, {0x3884800, 0x46c, 0x800})
        net/http/server.go:1594 +0x48
github.com/AlexxIT/go2rtc/cmd/mp4.handlerMP4.func1({0x740e48, 0x256ca50})
        github.com/AlexxIT/go2rtc/cmd/mp4/mp4.go:116 +0x6c
github.com/AlexxIT/go2rtc/pkg/core.(*Listener).Fire(...)
        github.com/AlexxIT/go2rtc/pkg/core/listener.go:16
github.com/AlexxIT/go2rtc/pkg/mp4.(*Consumer).AddTrack.func4(0x2972b40)
        github.com/AlexxIT/go2rtc/pkg/mp4/consumer.go:133 +0xa4
github.com/AlexxIT/go2rtc/pkg/core.(*Sender).HandleRTP.func1()
        github.com/AlexxIT/go2rtc/pkg/core/track.go:139 +0x64
created by github.com/AlexxIT/go2rtc/pkg/core.(*Sender).HandleRTP
        github.com/AlexxIT/go2rtc/pkg/core/track.go:135 +0x540
  1. bufio invalid memory access
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x3239bc]

goroutine 36 [running]:
bufio.(*Writer).Write(0x294a280, {0x2c7c000, 0x3749, 0x3add})
        bufio/bufio.go:668 +0xf0
net/http.(*response).write(0x29ac140, 0x3749, {0x2c7c000, 0x3749, 0x3add}, {0x0, 0x0})
        net/http/server.go:1636 +0x214
net/http.(*response).Write(0x29ac140, {0x2c7c000, 0x3749, 0x3add})
        net/http/server.go:1594 +0x48
github.com/AlexxIT/go2rtc/cmd/mp4.handlerMP4.func1({0x740e48, 0x289e0a0})
        github.com/AlexxIT/go2rtc/cmd/mp4/mp4.go:116 +0x6c
github.com/AlexxIT/go2rtc/pkg/core.(*Listener).Fire(...)
        github.com/AlexxIT/go2rtc/pkg/core/listener.go:16
github.com/AlexxIT/go2rtc/pkg/mp4.(*Consumer).AddTrack.func1(0x2d40680)
        github.com/AlexxIT/go2rtc/pkg/mp4/consumer.go:74 +0xc8
github.com/AlexxIT/go2rtc/pkg/h264.RTPDepay.func1(0x2aa0f00)
        github.com/AlexxIT/go2rtc/pkg/h264/rtp.go:92 +0x338
github.com/AlexxIT/go2rtc/pkg/core.(*Sender).HandleRTP.func1()
        github.com/AlexxIT/go2rtc/pkg/core/track.go:139 +0x64
created by github.com/AlexxIT/go2rtc/pkg/core.(*Sender).HandleRTP
        github.com/AlexxIT/go2rtc/pkg/core/track.go:135 +0x540
@AlexxIT AlexxIT added the bug Something isn't working label Apr 14, 2023
@AlexxIT
Copy link
Owner

AlexxIT commented Apr 14, 2023

Have your fix help with this problem?

@AlexxIT AlexxIT added this to the 1.3.2 milestone Apr 14, 2023
@yousong
Copy link
Contributor Author

yousong commented Apr 14, 2023

Have your fix help with this problem?

Yes. The issue can be illustrated with the following code, assuming go2rtc writes video/audio stream concurrently from 2 goroutines.

package main

import (
	"fmt"
	"net/http"
	"strings"
	"sync"
)

func main() {
	http.HandleFunc("/bar", func(w http.ResponseWriter, r *http.Request) {
		wg := &sync.WaitGroup{}
		const n = 10
		wg.Add(n)
		defer wg.Wait()
		w.WriteHeader(200)

		//mu := &sync.Mutex{}
		for i := 0; i < n; i++ {
			go func(i int) {
				defer wg.Done()
				//mu.Lock()
				//defer mu.Unlock()

				s := fmt.Sprintf("w%d|", i)
				s = strings.Repeat(s, 1024) + "\n"
				d := []byte(s)
				for {
					w.Write(d)
				}
			}(i)
		}
	})
	http.ListenAndServe(":8001", nil)
}

@yousong
Copy link
Contributor Author

yousong commented Apr 14, 2023

  1. bufio invalid memory access

It seems there are really two separate causes for the two observations above.

The invalid memory access panic is likely caused by ResponseWriter being de-initialized by net/http on http handler return.

Example code like below illustrates this.

http.HandleFunc("/boo", func(w http.ResponseWriter, r *http.Request) {
        w.WriteHeader(200)

        go func() {
                tick := time.NewTicker(time.Second)
                for range tick.C {
                        n, err := w.Write([]byte("c"))
                        log.Printf("n %d, err: %v", n, err)
                        if err == nil {
                                if f, ok := w.(http.Flusher); ok {
                                        f.Flush()
                                }
                        }
                }
        }()
        <-time.NewTimer(3 * time.Second).C
})

yousong added a commit to yousong/go2rtc that referenced this issue Apr 14, 2023
It seems net/http will deinit w on handler return and using it at that
time will cause invalid memory access panic.  This can be reproduced
when doing page refresh

Ref: AlexxIT#338
yousong added a commit to yousong/go2rtc that referenced this issue Apr 14, 2023
It seems net/http will deinit w on handler return and using it at that
time will cause invalid memory access panic.  This can be reproduced
when doing page refresh

Ref: AlexxIT#338
yousong added a commit to yousong/go2rtc that referenced this issue Apr 14, 2023
It seems net/http will deinit w on handler return and using it at that
time will cause invalid memory access panic.  This can be reproduced
when doing page refresh

Ref: AlexxIT#338
@AlexxIT AlexxIT removed this from the v1.3.2 milestone Apr 17, 2023
@AlexxIT
Copy link
Owner

AlexxIT commented Apr 17, 2023

Can you show how you config tl_ipc44gw_main?
Your changes are causing panic in my setup. And there is no problem without these changes.

@yousong
Copy link
Contributor Author

yousong commented Apr 17, 2023

Can you show how you config tl_ipc44gw_main? Your changes are causing panic in my setup. And there is no problem without these changes.

It's plain rtsp

log:
  level: trace
streams:
  tl_ipc44gw_main: rtsp://admin:xx@192.168.199.220:554/stream1

@AlexxIT
Copy link
Owner

AlexxIT commented Apr 17, 2023

Ok. I caught the problem once on my setup. And have returned the mutex. But not sure about your second patch yet.

@yousong
Copy link
Contributor Author

yousong commented Apr 17, 2023

The mutex is there for serializing writes to http.ResponseWriter. The example code above should prove that this serialization is necessary. I'd say in the least that it's very unlikely that the mutex should cause any new issue.

@AlexxIT AlexxIT changed the title Panic when accessing /api/stream.mp4 with both video and audio tracks enabled Fix panic for MP4 stream with audio after v1.3 Apr 17, 2023
@AlexxIT AlexxIT added this to the v1.3.2 milestone Apr 17, 2023
@AlexxIT
Copy link
Owner

AlexxIT commented Apr 17, 2023

@AlexxIT AlexxIT closed this as completed Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants