FLTK logo

[master] c555629 - Fix "Segfault if using very large selections" (issue #451)

FLTK matrix user chat room
(using Element browser app)   FLTK gitter user chat room   GitHub FLTK Project   FLTK News RSS Feed  
  FLTK Apps      FLTK Library      Forums      Links     Login 
 All Forums  |  Back to fltk.commit  ]
 
Previous Message ]Next Message ]

[master] c555629 - Fix "Segfault if using very large selections" (issue #451) "Albrecht Schlosser" Jul 01, 2022  
 
commit c5556291624eec58ed9de186474dfcc858dca691
Author:     Albrecht Schlosser <albrechts.fltk@online.de>
AuthorDate: Fri Jul 1 17:38:17 2022 +0200
Commit:     Albrecht Schlosser <albrechts.fltk@online.de>
CommitDate: Fri Jul 1 17:39:41 2022 +0200

    Fix "Segfault if using very large selections" (issue #451)
    
    - Fix reading the size (aka "lower bound") of selection data.
    
    - Use Fl::fatal() to terminate the process if memory for the selection
      (aka clipboard) data can't be allocated. This should rarely
      happen but if it does this is at least a "clean" exit and does
      not overwrite arbitrary data waiting for later errors that are
      hard to debug (as the old code would have done).
    
      Todo: find a better solution because this can be caused by
      another faulty process (the "selection owner"). It would be good
      if we could ignore the transfer rather than killing the process.
    
    - Continue processing the INCR protocol if another "unexpected" event
      is received. Such events can definitely happen but the current code
      can't deal with this because other events might cause recursions.
      Hence such events are currently ignored.
    
      Example: pressing and holding ctrl/v would trigger another clipboard
      transfer while we're still processing one.
    
    Todo: maybe process "other" events correctly while processing the INCR
      protocol. The current processing is done inside a function and would
      need to call fl_handle() with potential recursions, hence this would
      likely need major refactoring.

 src/Fl_x.cxx | 54 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git src/Fl_x.cxx src/Fl_x.cxx
index 648f44e..c07a69e 100644
--- src/Fl_x.cxx
+++ src/Fl_x.cxx
@@ -1029,11 +1029,18 @@ static long getIncrData(uchar* &data, const XSelectionEvent& selevent, size_t lo
   XEvent event;
   XDeleteProperty(fl_display, selevent.requestor, selevent.property);
   data = (uchar*)realloc(data, lower_bound);
+  if (!data) {
+    // fprintf(stderr, "[getIncrData:%d] realloc() FAILED, size = %ld\n", __LINE__, lower_bound);
+    Fl::fatal("Clipboard data transfer failed, size %ld too large.", lower_bound);
+  }
   for (;;) {
-    if (!getNextEvent(&event)) break;
-    if (event.type == PropertyNotify)
-    {
-      if (event.xproperty.state != PropertyNewValue) continue;
+    if (!getNextEvent(&event)) {
+      // This is unexpected but may happen if the sender (clipboard owner) no longer sends data
+      // fprintf(stderr, "[getIncrData:%d] Failed to get next event (timeout) *** break! ***\n", __LINE__);
+      break;
+    }
+    if (event.type == PropertyNotify) {
+      if (event.xproperty.state != PropertyNewValue) continue; // ignore PropertyDelete
       Atom actual_type;
       int actual_format;
       unsigned long nitems;
@@ -1048,14 +1055,38 @@ static long getIncrData(uchar* &data, const XSelectionEvent& selevent, size_t lo
         num_bytes = nitems * (actual_format / 8);
         offset += num_bytes/4;
         // slice_size += num_bytes;
-        if (total + num_bytes > (size_t)lower_bound) data = (uchar*)realloc(data, total + num_bytes);
-        memcpy(data + total, prop, num_bytes); total += num_bytes;
+        if (total + num_bytes > lower_bound) {
+          data = (uchar*)realloc(data, total + num_bytes);
+          if (!data) {
+            // fprintf(stderr, "[getIncrData():%d] realloc() FAILED, size = %ld\n", __LINE__, total + num_bytes);
+            Fl::fatal("Clipboard data transfer failed, size %ld too large.", total + num_bytes);
+          }
+        }
+        memcpy(data + total, prop, num_bytes);
+        total += num_bytes;
         if (prop) XFree(prop);
       } while (bytes_after != 0);
       // fprintf(stderr,"INCR data size:%ld\n", slice_size);
       if (num_bytes == 0) break;
     }
-    else break;
+    else {
+      // Unexpected next event. At this point we're handling the INCR protocol and can't deal with
+      // *some* other events due to potential recursions. We *could* call fl_handle(event) to handle
+      // *selected* other events but for the time being we ignore all other events!
+      // Handling the INCR protocol for very large data may take some time and multiple events.
+      // Interleaving "other" events are possible, for instance the KeyRelease event of the
+      // ctrl/v key pressed to insert the clipboard. This solution is not perfect but it can
+      // handle the INCR protocol with very large selections in most cases, although with potential
+      // side effects because other events may be ignored.
+      // See GitHub Issue #451: "Segfault if using very large selections".
+      // Note: the "fix" for Issue #451 is basically to use 'continue' rather than 'break'
+      // Debug:
+      // fprintf(stderr,
+      //   "[getIncrData:%d] getNextEvent() returned %d, not PropertyNotify (%d). Event ignored.\n",
+      //   __LINE__, event.type, PropertyNotify);
+
+      continue;
+    }
   }
   XDeleteProperty(fl_display, selevent.requestor, selevent.property);
   return (long)total;
@@ -1222,7 +1253,9 @@ int fl_handle(const XEvent& thisevent)
 
   case SelectionNotify: {
     static unsigned char* sn_buffer = 0;
-    if (sn_buffer) {XFree(sn_buffer); sn_buffer = 0;}
+    if (sn_buffer) {
+      free(sn_buffer); sn_buffer = 0;
+    }
     long bytesread = 0;
     if (fl_xevent->xselection.property) for (;;) {
       // The Xdnd code pastes 64K chunks together, possibly to avoid
@@ -1283,7 +1316,10 @@ int fl_handle(const XEvent& thisevent)
         return true;
       }
       if (actual == fl_INCR) {
-        bytesread = getIncrData(sn_buffer, xevent.xselection, *(long*)portion);
+        // an X11 "integer" (32 bit), the "lower bound" of the clipboard size (see ICCCM)
+        size_t lower_bound = (*(unsigned long *)portion) & 0xFFFFFFFF;
+        // fprintf(stderr, "[fl_handle:%d] INCR: lower_bound = %ld\n", __LINE__, lower_bound);
+        bytesread = getIncrData(sn_buffer, xevent.xselection, lower_bound);
         XFree(portion);
         break;
       }
Direct Link to Message ]
 
     
Previous Message ]Next Message ]
 
 

Comments are owned by the poster. All other content is copyright 1998-2024 by Bill Spitzak and others. This project is hosted by The FLTK Team. Please report site problems to 'erco@seriss.com'.