Re: [dev] Announcing a couple small X11 utilities

From: NRK <nrk_AT_disroot.org>
Date: Mon, 31 Jul 2023 15:44:00 +0600

Hi Yan,

On Sat, Jul 29, 2023 at 02:46:29PM +0200, Yan Doroshenko wrote:
> I've created a patch for sxot that adds a -m (--monitor) param that allows
> to select which monitor to capture in a multihead setup. Let me know what
> you think.

Thanks for the patch, I don't use a multimonitor setup to test it out
properly, but there are already a couple things which aren't too good.

+ int m[1];
+ str_to_int(str_from_cstr(argv[++i]), m);

str_to_int() does certain error checking (such as overflow) and returns
false in case of failure. That return value should not be ignored. It
should fatally error out if str_to_int() returns false.

It's also weird to use `int m[1]` instead of using `int m` and then
taking a pointer.

+ XRRScreenResources *screen;
+ screen = XRRGetScreenResources (x11.dpy, DefaultRootWindow(x11.dpy));

I'm not familiar with Xrander (and my local manpage is lacking
documentation for this function) but given that it returns a pointer, it
most likely needs to be error checked.

+ XRRCrtcInfo *crtc_info;
+ crtc_info = XRRGetCrtcInfo (x11.dpy, screen, screen->crtcs[m[0]]);

Same here, most likely needs error checking. But even more importantly:

        screen->crtcs[m[0]]

one can never assume anything about data that came from outside. There's
no guarantee that m[0] won't be bigger than the len of `screen->crtcs`.

I see that there's a `ncrtc` member, which likely contains the len of
`crtcs`. You should check to make sure that it doesn't exceed that.

If you compile with AddressSanitizer (see the "recommended debug build"
on the README) and input some absurdly high value, you'll notice the
buffer overflow:

        $ ./sxot -m 1024 >/dev/null
        =================================================================
        ==11432==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61a000002600 at pc 0x000000404271 bp 0x7ffe95aa74a0 sp 0x7ffe95aa7498

And even if you enter a valid value, ASan reports 2 leaks (output
cleaned up):

        $ ./sxot -m 0 >/dev/null
        SUMMARY: AddressSanitizer: 1296 byte(s) leaked in 2 allocation(s).

So something probably needs to be freed above.

- NRK
Received on Mon Jul 31 2023 - 11:44:00 CEST