Re: [dev] Announcing a couple small X11 utilities

[Yan Doroshenko]

Hello,





Regards,

Yan


On 7/31/23 11:44, NRK wrote:

I've addressed most of your concerns, please check the attached patch and let me know what else can be done.

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

diff --git a/sxot.c b/sxot.c
index de87126..094cd7b 100644
--- a/sxot.c
+++ b/sxot.c
@@ -31,6 +31,7 @@
 #include <X11/Xlib.h>
 #include <X11/Xutil.h>
 #include <X11/extensions/Xfixes.h>
+#include <X11/extensions/Xrandr.h>
 
 typedef uint8_t     u8;
 typedef uint32_t    u32;
@@ -207,7 +208,8 @@ main(int argc, char *argv[])
 		"usage: sxot [options]\n"
 		"Options:\n"
 		"  -g, --geom <x,y,w,h>   Capture the specified rectangle\n"
-		"  -c, --curosr           Capture the cursor also\n"
+		"  -c, --cursor           Capture the cursor also\n"
+		"  -m, --monitor n        Capture the specified monitor\n"
 	);
 	Str version = S(
 		"sxot " VERSION "\n"
@@ -237,6 +239,38 @@ main(int argc, char *argv[])
 			if (!parse_geom(opt.v, str_from_cstr(argv[++i]))) {
 				fatal(S("invalid geometry\n"));
 			}
+
+		} else if (str_eq(arg, S("--monitor")) || str_eq(arg, S("-m"))) {
+			int m;
+			if (!str_to_int(str_from_cstr(argv[++i]), &m)) {
+				fatal(S("invalid monitor number\n"));
+			}
+
+			int n;
+			XRRMonitorInfo *monitors;
+			monitors = XRRGetMonitors(x11.dpy, x11.root, true, &n);
+
+			free(monitors);
+
+			if (n == -1) {
+				fatal(S("get monitors failed\n"));
+			}
+			if (m >= n) {
+				fatal(S("no monitor with such index\n"));
+			}
+
+			XRRScreenResources *screen;
+			screen = XRRGetScreenResources(x11.dpy, DefaultRootWindow(x11.dpy));
+			XRRCrtcInfo *crtc_info;
+			crtc_info = XRRGetCrtcInfo(x11.dpy, screen, screen->crtcs[m]);
+
+			opt.x = crtc_info->x;
+			opt.y = crtc_info->y;
+			opt.h = crtc_info->height;
+			opt.w = crtc_info->width;
+
+			free(crtc_info);
+			free(screen);
 		} else if (str_eq(arg, S("--cursor")) || str_eq(arg, S("-c"))) {
 			opt.capture_cursor = true;
 		} else if (str_eq(arg, S("--help")) || str_eq(arg, S("-h"))) {

OpenPGP_signature.asc
Description: OpenPGP digital signature