kernel: fix reading of window command answers (xgap/package mode) - #6483
kernel: fix reading of window command answers (xgap/package mode)#6483RussWoodroofe wants to merge 4 commits into
Conversation
SyWinCmd reads a window handler's "@A<len>+<data>" reply into a fixed 8000-byte static buffer and returns that buffer for FuncWindowCmd to parse. Three things go wrong there: * A reply arriving in more than one read() is corrupted. Outside Cygwin the read loop never advances the destination pointer, so each chunk overwrites the start of the buffer, and it clobbers the byte count that the following un-escaping pass depends on; the resulting bad length can send FuncWindowCmd's parser past the end of the buffer. The same loops also mishandle a read that delivers nothing: 'len' is unsigned, so the -1 of a failed read is subtracted as a huge value and increases the count of bytes still wanted instead of retrying, and the 0 of an end of file leaves that count untouched, so the loop spins. * The un-escaping pass consumes two input bytes for an escape but counts only one, so a reply containing an escaped byte -- a literal '@', or a control character such as the newline of a multi-line string -- reads past the end of the payload and splices what follows into the result. * A reply longer than 8000 bytes overflows the buffer. These errors can be reproduced by (in xgap or Gap.app) running `gap> Query(Dialog("OKcancel", "test"));` and pasting in a string that is long or contains @'s. Since FuncWindowCmd already knows the length of each string before it reads it, it can allocate the result at that length and read the payload straight into it. SyWinCmd is therefore split into a send half, SyWinSendCmd, and small readers for the reply: the "@A<len>+" header, the kind of the next entry, an "I<int><sign>" entry, and the length and body of an "S<len>+<bytes>" entry, the last two separate so that the length is known before the string is read. The payload is read in bulk through a small fixed buffer, refilled in chunks bounded by the reply's declared length. Nothing reads past the end of a reply, no buffer holds a whole reply, and the length of a reply is no longer limited. EAGAIN is retried as in the other input loops in this file, and an end of file ends a read rather than spinning. Behaviour is otherwise unchanged: the wire protocol is the same, so window front ends need no changes, and a missing window handler or a malformed answer is still reported as Error( "window system: ", ... ). SyWinCmd itself is removed from the installed header sysfiles.h; the six readers that replace it are internal to the window protocol too. Prepared with the assistance of Claude Code, which analysed the code, proposed the restructuring and drafted the patch; reviewed, built and tested by me. Co-authored-by: Claude <noreply@anthropic.com>
fingolfin
added
kind: bug
|
Maybe ask Claude to also think about adding a testing rig, so that we can prevent this from regressing again? I wouldn't ask you to go through this hassle, but my experience is that Claude is quite add this. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6483 +/- ##
==========================================
- Coverage 79.08% 79.06% -0.03%
==========================================
Files 685 685
Lines 293789 293840 +51
Branches 8664 8674 +10
==========================================
- Hits 232357 232330 -27
- Misses 59631 59711 +80
+ Partials 1801 1799 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Claude had some reasonable ideas as to how to do some routine tests of package mode. I'm working on polishing -- will need my tokens to reset before I finish. Some things I am not certain about: |
In package mode 'WindowCmd' writes '@w<len>+<cmd>' and waits to read a
response. Nothing tested that reading until now:
tst/testinstall/kernel/gap.tst covers the argument checks and the "No
Window Handler Present" path, but not the parsing of an answer.
These tests supply the answer bytes directly, with no handler and no
second process. A test is fed to GAP on stdin, and the answer simply
follows the newline of the line that called 'WindowCmd'. With stdin
redirected, GAP reads the command only to the newline, and the answer is
still in the stream when the kernel asks for it. The kernel does not
interpret the three character command name, so "TST" is used as a
placeholder.
The tests need the '-p' flag on the command line, since there is no way
into package mode from within GAP. We add a new mechanism to
testspecial/run_gap.sh to support this: a test whose first line reads
'#GAPOPTS <options>' is run with those options added to GAP's command
line.
The tests are:
window-cmd-long-string.g strings longer than the kernel's refill buffer,
so that an answer is read in several pieces
window-cmd-entries.g the shapes an answer entry can take
window-cmd-truncated.g answers that come up short
The answer bytes are generated by dev/make-window-cmd-tests.sh.
Generation makes sense here since the lengths, the least significant
digit first digit runs and the escape counts all have to agree. It
takes the refill buffer size from SYS_WIN_BUF_SIZE rather than keeping
its own copy of it. The test generator is in dev/ because it is needed
only to change the tests, not to run them, and dev/ is not shipped in a
release.
README-window-cmd.md describes the answer format and what each test
covers. It is a separate file rather than an addition to README.md to
minimize change to the existing file. Other tests in that directory
appear to be undocumented and it would be worthwhile in the future to
fold README-window-cmd into the main README, and also to document the
other tests.
What is and is not covered
--------------------------
Three of these fail outright on the reader being replaced, and were
checked against it:
- a malformed header, which gave 'List Element: <list>[1] must have
an assigned value' rather than an error about the answer;
- a string entry claiming more bytes than the payload holds, which
was read out of the 8000 byte array the answer had been placed in;
- EOF part way through an answer, which spun forever, subtracting a
read() of 0 from the count of bytes outstanding without ever
reducing it. This one fails as a hang rather than as a diff, so
run_gap.sh now puts a timeout around GAP where the system has one.
The old un-escaping is also covered, by the mixed entries case. It
counted one output byte per escape while consuming two input bytes,
leaving garbage in the tail of its buffer; whether that was visible
depended on whether the entry parser reached it, and so on what the
previous answer had left in the buffer. Containing an escape was
therefore not on its own enough to provoke it -- the escape table case,
which is nothing but escapes, passes on the old reader.
The remaining cases test buffer refill on the new reader, including
splitting an @ encode at the first refill boundary. These tests also
pass on the old reader, which simply had a 8000 character buffer with no
refill.
Deliberately not covered:
- Answers over 8000 bytes. That was the size of the buffer the old
reader used. It is unlikely that the old reader is slotted back in,
and the other tests would flag if it were.
- Corruption of an answer split across several read() calls. When GAP
reads an answer in this test framework, typically the whole test is
already in the pipe. Testing this would require writing a handler
that writes incrementally.
- Answers that are not merely short but invalid, such as one carrying
no status entry, or one declaring a length too large to represent.
Those are a broken handler rather than something the kernel is
expected to survive gracefully.
- EAGAIN, and the '@y'/'@s' sync handshake. Both need a pseudo
terminal: input side '@' decoding lives only in syGetchTerm.
- The outgoing direction, '@w' framing and the escaping of the
command itself. run_gap.sh captures GAP's log rather than its
stdout, so the '@' stream is not visible to these tests.
|
@fingolfin , I added some tests in testspecial. The tests put GAP commands and window command lines in the same pipe. Edit: got to this quicker than expected, sorry for flurry of messages. User error, plus a fragility to wording of break messages. Testspecial now passes, no problem. One thing to flag: tst/testspecial/run_gap.sh gets some new features: the first line of a .g in that directory can specify options for gap (like -p) via a specially crafted comment line, and there is a timeout guard. Could handle differently if more appropriate. |
| const Char * bb; // pointer into the temporary | ||
| Char * b; // pointer into the temporary | ||
| UInt i; // loop variable | ||
| #ifdef SYS_IS_CYGWIN32 |
There was a problem hiding this comment.
I notice the CYGWIN stuff seems to get dropped in your new version. Just checking, is that because you believe it isn't needed any more, or you just aren't expecting GAP to support it any more?
There was a problem hiding this comment.
Tl;dr: the former.
In more detail: According to my best guess, the CYGWIN stuff is someone noticing broken code, and making an incomplete / local fix for it. (Indeed, this branch was closer to correct than the main branch.) The old CYGWIN stuff surely still had most of the same bugs as the main branch. The patch separates the variable len into syWinAnswerRemaining and local variable got (see syWinAnswerRaw), paralleling the CYGWIN ifdef. The new code should work on CYGWIN as near as I can tell. I don't have ready access to a Windows machine, and testing Cygwin and xgap together would require a specialized environment indeed.
3 participants
This PR addresses a long-standing bug in GAP's package mode.
Package mode is used by xgap and Gap.app to communicate with GAP. The routine that parses strings sent from xgap in response to a FuncWindowCmd call has several points of failure.
Details
The standing SyWinCmd routine reads a window handler's "@A+" reply into a fixed 8000-byte static buffer and returns that buffer for FuncWindowCmd to parse. Three things go wrong there:
A reply arriving in more than one read() is corrupted. Outside Cygwin (??) the read loop never advances the destination pointer, so each chunk overwrites the start of the buffer, and it clobbers the byte count that the following un-escaping pass depends on; the resulting bad length can send FuncWindowCmd's parser past the end of the buffer. The same loops also mishandle a read that delivers nothing: 'len' is unsigned, so the -1 of a failed read is subtracted as a huge value and increases the count of bytes still wanted instead of retrying, and the 0 of an end of file leaves that count untouched, so the loop spins.
The un-escaping pass consumes two input bytes for an escape but counts only one, so a reply containing an escaped byte -- a literal '@', or a control character such as the newline of a multi-line string -- reads past the end of the payload and splices what follows into the result.
A reply longer than 8000 bytes overflows the buffer.
These errors can be reproduced by (in xgap or Gap.app) running
gap> Query(Dialog("OKcancel", "test"));and pasting in a string that is long or contains @'s.Since FuncWindowCmd already knows the length of each string before it reads it, it can allocate the result at that length and read the payload straight into it. SyWinCmd is therefore split into a send half, SyWinSendCmd, and small readers for the reply: the "@A+" header, the kind of the next entry, an "I" entry, and the length and body of an "S+" entry, the last two separate so that the length is known before the string is read.
The payload is read in bulk through a small fixed buffer, refilled in chunks bounded by the reply's declared length. Nothing reads past the end of a reply, no buffer holds a whole reply, and the length of a reply is no longer limited. EAGAIN is retried as in the other input loops in this file, and an end of file ends a read rather than spinning.
Behaviour is otherwise unchanged: the wire protocol is the same, so window front ends need no changes, and a missing window handler or a malformed answer is still reported as Error( "window system: ", ... ). SyWinCmd itself is removed from the installed header sysfiles.h; the six readers that replace it are internal to the window protocol too.
The PR also has been updated to include tests, and a rig to drive limited tests of package mode. Technical details: the rig pipes to stdin a GAP WindowCmd command, followed immediately by a package mode answer. See the second commit message and the committed README-window-cmd.md for details.
Prepared with the assistance of Claude Code, which analysed the code, proposed (with a little guidance) the restructuring and drafted the patch; reviewed, built and tested (in both xgap and Gap.app) by me.
Text for release notes
see title