On 7/31/23 11:44, NRK wrote:
> 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
>
Howdy,
Thanks for the feedback. I'm not a C programmer in any way, shape or
form so it's completely expected for there to be issues with the code.
I'll do what I can about what you've mentioned and come back for
feedback or advice if you don't mind.
Thanks,
Yan
Received on Mon Jul 31 2023 - 11:51:56 CEST