Hi, This is a follow up of my post[1] originally on misc@ on x11/xsel crash.
Matthew Dempsky committed his fix for data argument to XChangeProperty() interface. Running with the updated port, I still ran into a crash (same crash based on backtrace). [short version] Attached diff fixes an issue where NUM_TARGETS in main() is incremented one too many times. The diff also plugs a memory leak in handle_targets() where a copy of supported_targets is created to be passed to change_property() -> XChangeProperty() but is not free()-ed. The malloc()/memcpy() may be entirely unnecessary. [longer version] NUM_TARGETS is what is passed in as nelements to XChangeProperty() and data is supported_targets, which is declared to have MAX_NUM_TARGETS (#define-d to 8) elements. Disclaimer: this is outside my expertise. Comment on supported_targets mentions all Atoms it has slots for. It does not specify the "NULL atom", which NUM_TARGETS is incremented for (the one too many). This leaves NUM_TARGETS equaling 9 at the end of initialization. Side note: the code uses an extra variable "s" as an incrementer as supported_targets is initialized, and it increments NUM_TARGETS along side. This is curious. I also added an assert(NUM_TARGETS <= MAX_NUM_TARGETS) in main() to catch mismatches should they happen in future code changes. I've been running this patch on amd64 for about a week and no issues so far. Note #1, make update-patches somehow changes patch-configure, even though it isn't changed. *shrug* Note #2, I contacted upstream author, both when Matthew posted his patch and a week ago when I found this mistake. I have not heard a peep from him. Thoughts? --patrick [1] http://marc.info/?l=openbsd-misc&m=140299560300878&w=2
Index: Makefile =================================================================== RCS file: /cvs/obsd/ports/x11/xsel/Makefile,v retrieving revision 1.8 diff -u -p -u -p -r1.8 Makefile --- Makefile 18 Jun 2014 20:49:15 -0000 1.8 +++ Makefile 30 Jun 2014 09:30:32 -0000 @@ -3,7 +3,7 @@ COMMENT= command-line program for managing X selection contents DISTNAME= xsel-1.2.0 -REVISION= 0 +REVISION= 1 CATEGORIES= x11 HOMEPAGE= http://www.vergenet.net/~conrad/software/xsel/ Index: patches/patch-configure =================================================================== RCS file: /cvs/obsd/ports/x11/xsel/patches/patch-configure,v retrieving revision 1.1 diff -u -p -u -p -r1.1 patch-configure --- patches/patch-configure 5 Sep 2009 08:55:51 -0000 1.1 +++ patches/patch-configure 30 Jun 2014 09:30:32 -0000 @@ -2,10 +2,9 @@ $OpenBSD$ -Wdeclaration-after-statement is gcc 4-only. ---- configure.orig Wed Jul 29 23:21:22 2009 -+++ configure Wed Jul 29 23:21:44 2009 -@@ -5879,9 +5879,9 @@ fi - +--- configure.orig Mon Mar 24 08:27:33 2008 ++++ configure Mon Jun 30 00:10:19 2014 +@@ -5880,7 +5880,7 @@ fi # Error out on compile warnings if test "x$ac_cv_c_compiler_gnu" = xyes ; then @@ -14,4 +13,3 @@ $OpenBSD$ fi # Checks for header files. - { echo "$as_me:$LINENO: checking for grep that handles long lines and -e" >&5 Index: patches/patch-xsel_c =================================================================== RCS file: /cvs/obsd/ports/x11/xsel/patches/patch-xsel_c,v retrieving revision 1.1 diff -u -p -u -p -r1.1 patch-xsel_c --- patches/patch-xsel_c 18 Jun 2014 20:49:15 -0000 1.1 +++ patches/patch-xsel_c 30 Jun 2014 09:30:32 -0000 @@ -1,24 +1,52 @@ $OpenBSD$ -Format "32" properties use "long", not "int", even on LP64 platforms. +- Format "32" properties use "long", not "int", even on LP64 platforms. +- ensure NUM_TARGETS does not exceed MAX_NUM_TARGETS. +- plug a memory leak in handle_targets() ---- xsel.c.orig Wed Jun 18 12:42:56 2014 -+++ xsel.c Wed Jun 18 12:43:16 2014 -@@ -630,7 +630,7 @@ wait_selection (Atom selection, Atom request_target) - } else if (target == incr_atom) { - /* Handle INCR transfers */ - retval = wait_incr_selection (selection, &event.xselection, -- *(int *)value); -+ *(long *)value); - keep_waiting = False; - } else if (target != utf8_atom && target != XA_STRING && - target != compound_text_atom && -@@ -1165,7 +1165,7 @@ change_property (Display * display, Window requestor, - Atom selection, Time time, MultTrack * mparent) +--- xsel.c.orig Mon Jun 30 00:10:19 2014 ++++ xsel.c Mon Jun 30 00:20:50 2014 +@@ -15,6 +15,7 @@ + #include "config.h" + #endif + ++#include <assert.h> + #include <stdio.h> + #include <stdlib.h> + #include <stdarg.h> +@@ -1300,14 +1301,16 @@ handle_targets (Display * display, Window requestor, A + Atom selection, Time time, MultTrack * mparent) { - XSelectionEvent ev; -- int nr_bytes; -+ long nr_bytes; - IncrTrack * it; + Atom * targets_cpy; ++ HandleResult r; + + targets_cpy = malloc (sizeof (supported_targets)); + memcpy (targets_cpy, supported_targets, sizeof (supported_targets)); + +- return +- change_property (display, requestor, property, XA_ATOM, 32, ++ r = change_property (display, requestor, property, XA_ATOM, 32, + PropModeReplace, (unsigned char *)targets_cpy, + NUM_TARGETS, selection, time, mparent); ++ free(targets_cpy); ++ return r; + } + + /* +@@ -2078,7 +2081,6 @@ main(int argc, char *argv[]) + + /* Get the NULL atom */ + null_atom = XInternAtom (display, "NULL", False); +- NUM_TARGETS++; + + /* Get the TEXT atom */ + text_atom = XInternAtom (display, "TEXT", False); +@@ -2096,6 +2098,8 @@ main(int argc, char *argv[]) + + supported_targets[s++] = XA_STRING; + NUM_TARGETS++; ++ ++ assert(NUM_TARGETS <= MAX_NUM_TARGETS); - print_debug (D_TRACE, "change_property ()"); + /* Get the COMPOUND_TEXT atom. + * NB. We do not currently serve COMPOUND_TEXT; we can retrieve it but
