Finding #5 I left the whole analysis as I really don't know the code but the early return in -endEvent:for: is suspicious.
Hope this helps, Philippe === Problem: When an event handler runs the run loop again (a modal loop, or waiting for a reply in another mode), the nested poll may see and handle an event which the outer poll also saw and has not handled yet. -endEvent:for: is how the nested poll tells the outer contexts to drop such an event (it removes the watcher from their descriptor maps), but the run loop reorganisation of d991cdaf7 (2026-10-19) added a bare 'return;' at its start, disabling it. The outer poll then called the watcher again for an event which had already been handled: a handler reading from a descriptor blocks (a blocking descriptor) or reads nothing (a non-blocking one). In the new test, 13 of 20 rounds called a watcher a second time. Also, for trigger watchers -endEvent:for: removed the entry from the outer poll's _trigger array while that poll goes through it by index, so entries shifted and the outer poll could call the watcher it was handling a second time and skip another one. This was already the case before d991cdaf7 (in Source/GSRunLoopCtxt.m too). Severity: medium. Events handled twice, and a hang when the second read is on a blocking descriptor, in programs which run the loop from an event handler. Discovery: found while writing a test for the run loop's poll() index (Tests/base/NSRunLoop/watcher-changes.m, in a later patch), whose nested run hung in a second read of a pipe. Fix: remove the early return; clear trigger entries (set them to nil) instead of removing them, in Source/unix/GSRunLoopCtxt.m and in the base class's Source/GSRunLoopCtxt.m (used by win32), and skip cleared entries in the trigger loops of the poll(), select() and win32 versions (win32 by inspection only, not built or tested). Test: Tests/base/NSRunLoop/nested-events.m (1 of its 3 tests fails before the fix). The NSRunLoop, NSFileHandle, NSStream, NSTask, NSConnection, NSThread, NSURLConnection and NSOperation tests pass unchanged. The trigger part has no test of its own. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- diff --git a/Source/GSRunLoopCtxt.m b/Source/GSRunLoopCtxt.m index ef6d5d3f0..08232c49c 100644 --- a/Source/GSRunLoopCtxt.m +++ b/Source/GSRunLoopCtxt.m @@ -138,7 +138,10 @@ static const NSMapTableValueCallBacks WatcherMapValueCallBacks = if (item.obj == (id)watcher) { - GSIArrayRemoveItemAtIndex(_trigger, i); + /* Clear the entry rather than removing it: the outer poll + * may be going through the array by index. + */ + GSIArraySetItemAtIndex(_trigger, (GSIArrayItem)(id)nil, i); return; } } diff --git a/Source/unix/GSRunLoopCtxt.m b/Source/unix/GSRunLoopCtxt.m index d9d572eb1..9200053a3 100644 --- a/Source/unix/GSRunLoopCtxt.m +++ b/Source/unix/GSRunLoopCtxt.m @@ -101,7 +101,6 @@ typedef struct { - (void) endEvent: (void*)data for: (GSRunLoopWatcher*)watcher { -return; if (completed == NO) { unsigned i = GSIArrayCount(_trigger); @@ -112,7 +111,10 @@ return; if (item.obj == (id)watcher) { - GSIArrayRemoveItemAtIndex(_trigger, i); + /* Clear the entry rather than removing it: the outer poll + * may be going through the array by index. + */ + GSIArraySetItemAtIndex(_trigger, (GSIArrayItem)(id)nil, i); return; } } @@ -486,7 +488,9 @@ static void setPollfd(int fd, int event, GSRunLoopCtxtUnix *ctxt) GSRunLoopWatcher *watcher; watcher = (GSRunLoopWatcher*)GSIArrayItemAtIndex(_trigger, count).obj; - if (watcher->_invalidated == NO) + /* nil if a nested poll has already handled it (see -endEvent:for:). + */ + if (watcher != nil && watcher->_invalidated == NO) { i = [contexts count]; while (i-- > 0) @@ -990,7 +994,9 @@ selectFd(int fd, fd_set *set, int *fdEnd) GSRunLoopWatcher *watcher; watcher = (GSRunLoopWatcher*)GSIArrayItemAtIndex(_trigger, count).obj; - if (watcher->_invalidated == NO) + /* nil if a nested poll has already handled it (see -endEvent:for:). + */ + if (watcher != nil && watcher->_invalidated == NO) { i = [contexts count]; while (i-- > 0) diff --git a/Source/win32/GSRunLoopCtxt.m b/Source/win32/GSRunLoopCtxt.m index 9faa7b38b..eea1a050e 100644 --- a/Source/win32/GSRunLoopCtxt.m +++ b/Source/win32/GSRunLoopCtxt.m @@ -534,7 +534,9 @@ GSRunLoopWatcher *watcher; watcher = (GSRunLoopWatcher*)GSIArrayItemAtIndex(_trigger, count).obj; - if (watcher->_invalidated == NO) + /* nil if a nested poll has already handled it (see -endEvent:for:). + */ + if (watcher != nil && watcher->_invalidated == NO) { if (debug) { diff --git a/Tests/base/NSRunLoop/nested-events.m b/Tests/base/NSRunLoop/nested-events.m new file mode 100644 index 000000000..830060f50 --- /dev/null +++ b/Tests/base/NSRunLoop/nested-events.m @@ -0,0 +1,133 @@ +/* + * When an event handler runs the run loop again (in another mode, as a + * modal loop does) and that nested run handles an event which the outer + * poll also saw, the outer poll must not call the watcher for it again + * afterwards: the nested run tells the outer contexts with + * -endEvent:for:. The descriptors are non-blocking, so a second call + * finds nothing to read instead of blocking. + */ +#import <Foundation/Foundation.h> +#import "Testing.h" +#include <fcntl.h> +#include <unistd.h> + +static NSString *nested = @"GSTestNestedMode"; + +@interface Handler : NSObject <RunLoopEvents> +{ +@public + int shared[2]; // Watched in both modes + int trigger[2]; // Watched in the default mode: runs the nested loop + int sharedReads; // Bytes read from shared + int emptyCalls; // Calls with nothing to read + int nestedRuns; +} +@end + +@implementation Handler +- (void) receivedEvent: (void*)data + type: (RunLoopEventType)type + extra: (void*)extra + forMode: (NSString*)mode +{ + int fd = (int)(intptr_t)data; + char c; + + if (read(fd, &c, 1) != 1) + { + emptyCalls++; + return; + } + if (fd == shared[0]) + { + sharedReads++; + } + else if (fd == trigger[0] && [mode isEqual: NSDefaultRunLoopMode]) + { + NSDate *limit = [NSDate dateWithTimeIntervalSinceNow: 0.3]; + + nestedRuns++; + while (sharedReads == 0 && [limit timeIntervalSinceNow] > 0) + { + [[NSRunLoop currentRunLoop] runMode: nested + beforeDate: [NSDate dateWithTimeIntervalSinceNow: 0.05]]; + } + } +} +@end + +static void +nonBlocking(int fd) +{ + fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK); +} + +int main() +{ + NSAutoreleasePool *arp = [NSAutoreleasePool new]; + NSRunLoop *rl = [NSRunLoop currentRunLoop]; + Handler *h = AUTORELEASE([Handler new]); + int round; + int once = 0; + int empty = 0; + int runs = 0; + + pipe(h->shared); + pipe(h->trigger); + nonBlocking(h->shared[0]); + nonBlocking(h->trigger[0]); + [rl addEvent: (void*)(intptr_t)h->shared[0] type: ET_RDESC watcher: h + forMode: NSDefaultRunLoopMode]; + [rl addEvent: (void*)(intptr_t)h->trigger[0] type: ET_RDESC watcher: h + forMode: NSDefaultRunLoopMode]; + [rl addEvent: (void*)(intptr_t)h->shared[0] type: ET_RDESC watcher: h + forMode: nested]; + + START_SET("an event handled by a nested run") + + /* Both descriptors are readable when the outer poll is made, so it + * sees both; depending on which it handles first, the shared one is + * handled by the outer run or by the nested one, but only once. + */ + for (round = 0; round < 20; round++) + { + NSDate *limit = [NSDate dateWithTimeIntervalSinceNow: 1.0]; + + h->sharedReads = 0; + h->emptyCalls = 0; + h->nestedRuns = 0; + write(h->shared[1], "x", 1); + write(h->trigger[1], "x", 1); + while ((h->sharedReads == 0 || h->nestedRuns == 0) + && [limit timeIntervalSinceNow] > 0) + { + [rl runMode: NSDefaultRunLoopMode + beforeDate: [NSDate dateWithTimeIntervalSinceNow: 0.05]]; + } + /* Let the outer loop finish anything it still had to handle. */ + [rl runMode: NSDefaultRunLoopMode + beforeDate: [NSDate dateWithTimeIntervalSinceNow: 0.05]]; + if (h->sharedReads == 1) + { + once++; + } + empty += h->emptyCalls; + runs += h->nestedRuns; + } + PASS(runs == 20, "the nested loop ran each time") + PASS(once == 20, "the shared descriptor was read each time") + PASS(empty == 0, + "no watcher was called again for an event already handled (%d calls)", + empty) + + END_SET("an event handled by a nested run") + + [rl removeEvent: (void*)(intptr_t)h->shared[0] type: ET_RDESC + forMode: NSDefaultRunLoopMode all: YES]; + [rl removeEvent: (void*)(intptr_t)h->trigger[0] type: ET_RDESC + forMode: NSDefaultRunLoopMode all: YES]; + [rl removeEvent: (void*)(intptr_t)h->shared[0] type: ET_RDESC + forMode: nested all: YES]; + [arp release]; + return 0; +} -- 2.39.5
