Panic in xv.QueryAdaptors

[gen2brain]

I am trying to use the XVideo extension, and the input I am getting is an image.YCbCr, so being able to directly copy YUV planes and use xv.ShmPutImage would be really nice, and I guess I could also use hardware scaling then.

I am getting panic when I query for adaptors:

panic: runtime error: slice bounds out of range [5388:45]

goroutine 1 [running]:
github.com/jezek/xgb/xv.AdaptorInfoRead({0xc0000b204b, 0x2d, 0x2d}, 0xc0000b4038)
    github.com/jezek/xgb@v1.0.1/xv/xv.go:83 +0x294
github.com/jezek/xgb/xv.AdaptorInfoReadList({0xc0000b2020, 0x58, 0x58}, {0xc0000b4000, 0x2, 0xc00018c060?})
    github.com/jezek/xgb@v1.0.1/xv/xv.go:93 +0x5d
github.com/jezek/xgb/xv.queryAdaptorsReply({0xc0000b2000, 0x78, 0x78})
    github.com/jezek/xgb@v1.0.1/xv/xv.go:2033 +0x1a8
github.com/jezek/xgb/xv.QueryAdaptorsCookie.Reply({0xc000122000?})
    github.com/jezek/xgb@v1.0.1/xv/xv.go:2011 +0x65
main.main()
    example/main.go:54 +0x471

Here is a simple example, Init and QueryExtension are successful. I tried to query before and after the window is mapped, but it makes no difference.

package main

import (
    "fmt"
    "os"

    "github.com/jezek/xgb"
    "github.com/jezek/xgb/xproto"
    "github.com/jezek/xgb/xv"
)

func main() {
    X, err := xgb.NewConn()
    if err != nil {
        fmt.Println(err)
        os.Exit(1)
    }
    defer X.Close()

    useXv := true
    err = xv.Init(X)
    if err != nil {
        useXv = false
    }
    fmt.Println(useXv)

    setup := xproto.Setup(X)
    screen := setup.DefaultScreen(X)

    r, err := xv.QueryExtension(X).Reply()
    if err != nil {
        fmt.Println(err)
        os.Exit(1)
    }
    fmt.Println(r.Major, r.Minor)

    window, err := xproto.NewWindowId(X)
    if err != nil {
        fmt.Println(err)
        os.Exit(1)
    }

    xproto.CreateWindow(X, screen.RootDepth, window, screen.Root, 0, 0, 640, 480, 1,
        xproto.WindowClassInputOutput, screen.RootVisual,
        xproto.CwBackPixmap|xproto.CwBackPixel|xproto.CwEventMask,
        []uint32{
            xproto.BackPixmapParentRelative,
            screen.BlackPixel,
            xproto.EventMaskKeyPress | xproto.EventMaskExposure | xproto.EventMaskStructureNotify,
        },
    )
    defer xproto.DestroyWindow(X, window)

    reply, err := xv.QueryAdaptors(X, window).Reply()
    if err != nil {
        fmt.Println(err)
        os.Exit(1)
    }
    fmt.Println(reply.NumAdaptors)

    xproto.MapWindow(X, window)

    for {
        X.WaitForEvent()
    }
}

[puellanivis]

Hmm… it would seem there’s some sort of invalid data in the byte slice that it’s parsing. These lines of code are problematic:

        byteString := make([]byte, v.NameSize)
        copy(byteString[:v.NameSize], buf[b:])
        v.Name = string(byteString)
        b += int(v.NameSize)

It should probably be:

        byteString := make([]byte, v.NameSize)
        n := copy(byteString[:v.NameSize], buf[b:])
        if n != v.NameSize {
            // handle error condition here.
        }
        v.Name = string(byteString)
        b += int(v.NameSize)

Unfortunately, it looks like the whole function was converted from C/C++ and isn’t all that Go idiomatic. So, there’s nothing really that could be done here to avoid the error, except panic… and it’s already doing that.

[jezek]

Hello @gen2brain , I run your example and got no error.

$ go run main.go
true
2 2
1
^Csignal: interrupt

I can't help if I can't reproduce. What's your setup? How can I get it to crash?

[gen2brain]

Hmm, I have a working XVideo in other apps like MPV, etc. I see that the NumAdaptors is 1 in your case, in my, it would be 2 if it worked:

xvinfo | grep Adaptor
  Adaptor #0: "Intel(R) Textured Video"
  Adaptor #1: "Intel(R) Video Sprite"

I will try another system that hopefully has only one adaptor if that is related at all. Thanks.

[jezek]

@gen2brain Yes,I have only one adaptor. Also I'm on wayland, so it's using XWayland in the background probably.

$ xvinfo | grep Adaptor
  Adaptor #0: "glamor textured video"

PS: I checked, I don't have 2 adaptors on my PC either. Sorry I can't help.

[gen2brain]

Ok, definitely it is triggered when there are 2 adaptors. I forced in code to loop only 1 instead of len(dest) and it doesn't crash anymore. I added some debug print statements:

queryAdaptorsReply
Sequence 5
Length 22
NumAdaptors 2

AdaptorInfoReadList
buf len 88
buf [78 0 0 0 23 0 64 0 1 0 17 0 73 110 116 101 108 40 82 41 32 84 101 120 116 117 114 101 100 32 86 105 100 101 111 0 32 0 0 0 24 247 46 116 142 0 0 0 21 0 1 0 1 0 17 0 73 110 116 101 108 40 82 41 32 86 105 100 101 111 32 83 112 114 105 116 101 0 0 0 32 0 0 0 24 247 46 116]

AdaptorInfoRead
BaseId 78
NameSize 23
NumPorts 64
NumFormats 1
Type 17
read 43

AdaptorInfoRead
BaseId 36468
NameSize 5376
NumPorts 256
NumFormats 256
Type 0

panic: runtime error: slice bounds out of range [5388:45]

The numbers for the second adapter are totally out of range. What I also noticed is that "number of formats" is not correct even for the first adaptor, xvinfo reports this:

xvinfo | grep "ports\|format"
    number of ports: 64
    Number of image formats: 7
    number of ports: 1
    Number of image formats: 6

But in the first output, there are 64 ports but only one format. Can you check if that is the case also for you?

[gen2brain]

Ok, found it.

--- xgb.orig/xv/xv.go   2022-10-29 19:58:03.271498437 +0200
+++ xgb/xv/xv.go    2022-10-29 20:01:00.603296923 +0200
@@ -77,7 +77,7 @@
        b += int(v.NameSize)
    }

-   b += 0 // padding
+   b += 1 // padding

    v.Formats = make([]Format, v.NumFormats)
    b += FormatReadList(buf[b:], v.Formats)

With this change it looks like this:

queryAdaptorsReply
Sequence 5
Length 22
NumAdaptors 2

AdaptorInfoReadList
buf len 88

AdaptorInfoRead
len 88
BaseId 78
NameSize 23
NumPorts 64
NumFormats 1
Type 17
b 12
Name Intel(R) Textured Video
b 44
read 44

AdaptorInfoRead
len 44
BaseId 142
NameSize 21
NumPorts 1
NumFormats 1
Type 17
b 12
Name Intel(R) Video Sprite
b 42
read 42

I am just worried about the NumFormats, not sure what is problem there I hope I won't be needing it. I can send PR for this change if you want, whatever is easier for you, thanks.

[aarzilli]

The code in question is autogenerated from the xcb-proto xml files, the version that BurntSushi used didn't have padding descriptors, I wrote the heuristic to add the padding but it is sometimes wrong near list entries because X11 is inconsistent with the padding there and the only way to do it correctly is to have the padding described explicitly in the xml sources.

Five years ago I change the code generation script to use xcb-proto-1.12 and regenerated all the files but the changes were never merged. Realistically someone should just regenerate everything from xcb-proto 1.14.

[gen2brain]

@aarzilli Thanks a lot for the info and for the patch. I just opened a PR https://github.com/jezek/xgb/pull/10 with your patch and with all the files regenerated. Everything I tried now with xv worked, with just the padding patch above it is again not usable, the Info and Format list are empty, etc.

[jezek]

@aarzilli Hello, you write I wrote the heuristic to add the padding but it is sometimes wrong near list entries... and then Five years ago I change the code generation script to use xcb-proto-1.12... and I don't know, is it sometimes wrong, or have you fixed it with the proposed changes?

[aarzilli]

• xgb originally used xcb-proto 1.11 (actually it could have been an even older release)
• xcb-proto prior to 1.12 did not have explicit alignment padding entries before list entries
• in 2014 I wrote a heuristic to add them, but it was just a heuristic and sometimes it is wrong, because X11 is inconsistent with alignment padding before lists
• five years ago I removed the heuristic and updated xgbgen to read the alignment padding entries added to xcb-proto 1.12, then regenerated everything. This was never merged
• I can't reproduce this problem so I can't confirm that it is the cause, but it is likely that it would. It also seems that it fixed the problem for @gen2brain (if I understand correctly)
• in general it would be a good idea to remove my heuristic, update xgbgen to use alignment padding entries and regenerate everything from a more recent version of xcb-proto, I would suggest going all the way up to 1.14. Even if it doesn't fix it, it will rule out the possibility that alignment padding is the problem.

[jezek]

@aarzilli Thank you for your explanation. @gen2brain says that he used your patch with the latest xcb-proto and everything works out for him. Do you have a reason, why you suggest 1.14 and not the latest? (I'm a little bit in the dark here in the code generation and xcb-proto versions)

[jezek]

... I just opened a PR #10 with your patch and with all the files regenerated. Everything I tried now with xv worked, with just the padding patch above it is again not usable, the Info and Format list are empty, etc.

@gen2brain Just to be sure. Do I understand correctly? You say that your (own) patch you suggested above does not work, but @aarzilli patch with regenerated code with the xcb-proto 1.15 in PR #10 works fine for you?

[gen2brain]

@jezek Yes, that is correct, with the @aarzilli patch and regenerated code everything works perfectly.

[aarzilli]

why you suggest 1.14 and not the latest?

I thought it was the latest because that's the last one they listed on their freedesktop website: https://xcb.freedesktop.org/

[jezek]

The PR #10 is merged, I'm closing this issue. Thank you for your cooperation.