Details

Reviewers

jhb
markj
rang_acm.org

Group Reviewers

manpages

Commits

rG3e5e9939cda3: ddb: enable the use of ^C and ^S/^Q

Summary

This lets one interrupt DDB's output, which is useful if paging is
disabled and the output device is slow.

This follows a previous implementation in svn r311952 / git
5fddef79998678d256ba30316353393b4d8ebb32 which was reverted because it
broke DDB type-ahead.

Now, try this again, but with a 512-byte type-ahead buffer. While there
is buffer space, control input is handled and non-control input is
buffered. When the buffer is exhausted, the default is to print a
warning and drop further non-control input in order to continue handling
control input. sysctl debug.ddb.prioritize_control_input can be set to
0 to instead preserve all input but lose immediate handling of control
input. This could for example effect pasting of a large script into the
ddb console.

Test Plan

When multiple commands are pasted into the console, they are all
buffered and executed as normal unless the input exceeds the buffer
size.

set $lines=0
alltrace
x/s version

When ^C is sent to a long-running command, the command is terminated and
the debugger returns to the db> prompt.

set $lines=0
alltrace
^C

When type ahead console input exceeds the buffer size, further input is
dropped but the console still responds to ^C/^S/^Q.

set $lines=0
alltrace
x/s version
x/s version
x/s version
x/s version
x/s version
x/s version
x/s version
x/s version
x/s version
x/s version
x/s version
...
x/s version
^C

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rlibby created this revision.Feb 15 2021, 2:21 AM

Herald added a subscriber: imp. · View Herald TranscriptFeb 15 2021, 2:21 AM

rlibby requested review of this revision.Feb 15 2021, 2:21 AM

Harbormaster completed remote builds in B37007: Diff 83902.Feb 15 2021, 2:21 AM

rlibby added inline comments.Feb 15 2021, 2:28 AM

sys/ddb/db_input.c
61	A guess at a size that is small enough not to be too wasteful and large enough that most people pasting multiple commands into the db> prompt won't notice.
420–427	Maybe this deserves a comment? For some commands, like alltrace, db_error() is not enough because they have their own jmpbufs.

Couple of comments but I don't know a lot about the character pipeline so I'm not sure how much good they will do.

sys/ddb/db_input.c
61	I'd have selected this or 1024.... but I'd think that fifo sizes would limit this to tens of bytes plus overflow unless hardware flow control was setup...
332	This is an excellent choke point for having a variable to disable this feature, which would allow the script pasters turn amuck.

imp feedback: provide a chicken switch

Harbormaster completed remote builds in B37010: Diff 83911.Feb 15 2021, 7:28 AM

In D28676#641723, @imp wrote:

Couple of comments but I don't know a lot about the character pipeline so I'm not sure how much good they will do.

Thanks for the feedback.

sys/ddb/db_input.c
61	I'm pretty ignorant on the console side so I'll defer to your knowledge there. Let me know if your preference is to bump it up. Empirically, I can hit this 512 byte limit with a large paste on a qemu VM at least.
332	Thanks. I added a switch near the cncheckc() instead so that ^C will usually still work with the switch disabled, at least until the input buffer is exhausted, which most of the time it won't be. I've made it a sysctl for now, but it could also be a ddb variable if you think that would be useful (but on the other hand, someone who knew the voodoo could just write the variable with ddb...). I struggled to name the switch and am open to suggestions there.

So if one were to programatically send a sequence of commands, separated by newlines, to ddb, does this change mean that after the first command begins executing, the echoed characters from subsequent commands will be interleaved with output from the first command?

syzkaller does something like this for what it's worth. See the end of this log for an example: https://syzkaller.appspot.com/text?tag=CrashLog&x=14743a37900000

sys/ddb/db_input.c
66	Probably this should be RWTUN (really that should be the default). I think this probably belongs under debug.ddb? As-is this creates an OID directly under debug.
440	You might consider documenting this in ddb.4, under DESCRIPTION or HINTS.

In D28676#642635, @markj wrote:

So if one were to programatically send a sequence of commands, separated by newlines, to ddb, does this change mean that after the first command begins executing, the echoed characters from subsequent commands will be interleaved with output from the first command?

No, I don't think this should change interleaving. In my first test scenario above (paste "alltrace\nx/s version") the result is that after alltrace runs the prompt fills with "db> x/s version". The reason is that the character echoing is handled by ddb in db_inputchar(). So what happens is the new code consumes characters from the console and buffers them without printing them.

I'll be happy to do additional testing on that though.

syzkaller does something like this for what it's worth. See the end of this log for an example: https://syzkaller.appspot.com/text?tag=CrashLog&x=14743a37900000

Do you have a pointer to source for how syzkaller scripts ddb?

So, yeah, I don't want to break this kind of thing. On the other hand I want it to be possible to break out of a situation where a tool has executed some kind of command that would take forever to complete and could result in loss of the debugger. What's in the log you linked should fit in the 512 byte buffer, but the point stands if someone had a bigger script like that. One possibility could be just to default to the prioritize_control_input=0 behavior, so the break ability is only lost for large scripts. Another could be just a big bump in the buffer size (2k? 4k?). Thoughts?

sys/ddb/db_input.c
66	Will do.
440	Will consider how to word this.

In D28676#642677, @rlibby wrote:

In D28676#642635, @markj wrote:

So if one were to programatically send a sequence of commands, separated by newlines, to ddb, does this change mean that after the first command begins executing, the echoed characters from subsequent commands will be interleaved with output from the first command?

No, I don't think this should change interleaving. In my first test scenario above (paste "alltrace\nx/s version") the result is that after alltrace runs the prompt fills with "db> x/s version". The reason is that the character echoing is handled by ddb in db_inputchar(). So what happens is the new code consumes characters from the console and buffers them without printing them.

Ok, that sounds good enough.

I'll be happy to do additional testing on that though.

syzkaller does something like this for what it's worth. See the end of this log for an example: https://syzkaller.appspot.com/text?tag=CrashLog&x=14743a37900000

Do you have a pointer to source for how syzkaller scripts ddb?

It's here: https://github.com/google/syzkaller/blob/master/vm/vmimpl/freebsd.go

w is an input stream for the VM's console. With bhyve it'll be the stdin of the bhyve process: https://github.com/google/syzkaller/blob/master/vm/bhyve/bhyve.go#L204
(See the DiagnoseFreeBSD call in the same file.)

So, yeah, I don't want to break this kind of thing. On the other hand I want it to be possible to break out of a situation where a tool has executed some kind of command that would take forever to complete and could result in loss of the debugger. What's in the log you linked should fit in the 512 byte buffer, but the point stands if someone had a bigger script like that. One possibility could be just to default to the prioritize_control_input=0 behavior, so the break ability is only lost for large scripts. Another could be just a big bump in the buffer size (2k? 4k?). Thoughts?

So ddb does support a scripting facility, whereby you can run something like

db> script test= show pcpu; ps
db> run test
-- "show pcpu" output, "ps" output --

So there is at least a workaround for this kind of problem, though there you are limited by TOK_STRING_SIZE, I believe.

I note that you apparently can't just run show pcpu; ps: everything after the semicolon is apparently ignored. It might be nice to fix that. There also seems to be a bug with the script command in that the first character after = is clipped, contrary to ddb.4. For instance:

db> script lockinfo=show alllocks; show lockedvnods
db> run lockinfo
db:0:lockinfo> how alllocks
No such command; use "help" to list available commands
db:0:lockinfo>  show lockedvnods
Locked vnodes

Anyway, since it's possible to submit multiple commands with a single line I'm inclined to think that the 512 byte limit is fine. Could we print warning the first time the buffer is filled, to give anyone affected a breadcrumb?

In D28676#642760, @markj wrote:
In D28676#642677, @rlibby wrote:

So, yeah, I don't want to break this kind of thing. On the other hand I want it to be possible to break out of a situation where a tool has executed some kind of command that would take forever to complete and could result in loss of the debugger. What's in the log you linked should fit in the 512 byte buffer, but the point stands if someone had a bigger script like that. One possibility could be just to default to the prioritize_control_input=0 behavior, so the break ability is only lost for large scripts. Another could be just a big bump in the buffer size (2k? 4k?). Thoughts?

So ddb does support a scripting facility, whereby you can run something like
db> script test= show pcpu; ps
db> run test
-- "show pcpu" output, "ps" output --
So there is at least a workaround for this kind of problem, though there you are limited by TOK_STRING_SIZE, I believe.

Yeah, and #define DB_MAXSCRIPTLEN 128.

I note that you apparently can't just run show pcpu; ps: everything after the semicolon is apparently ignored. It might be nice to fix that.

Hmm, I'm not sure how easy this is to fix. You want to be able to parse both show pcpu; ps and script test=show pcpu; ps, and presumably the latter should result in the test script being show pcpu; ps. We could make the script command special...

There also seems to be a bug with the script command in that the first character after = is clipped, contrary to ddb.4. For instance:
db> script lockinfo=show alllocks; show lockedvnods
db> run lockinfo
db:0:lockinfo> how alllocks
No such command; use "help" to list available commands
db:0:lockinfo>  show lockedvnods
Locked vnodes

I see the problem here, we can take it to another review. In short, the lexer sees = and looks for ==, then does db_unread_char(), which isn't compatible with db_get_line().

Anyway, since it's possible to submit multiple commands with a single line I'm inclined to think that the 512 byte limit is fine. Could we print warning the first time the buffer is filled, to give anyone affected a breadcrumb?

Yeah, will do.

markj feedback:

Rename the sysctl and make it tunable
output a warning when input is dropped
man page blurb

Also:

use unsigned types for indexes that in principle could wrap
improve comments

Herald added a reviewer: manpages. · View Herald TranscriptFeb 17 2021, 5:34 AM

Harbormaster completed remote builds in B37085: Diff 84066.Feb 17 2021, 5:34 AM

rlibby edited the summary of this revision. (Show Details)Feb 17 2021, 5:36 AM

Looks good to me, just a couple of minor notes.

share/man/man4/ddb.4
1576	New sentences should start on new lines, ditto below.
sys/ddb/db_input.c
449	I think I'd expect ^C to flush the typeahead buffer. Did you consider doing that here?

rlibby added inline comments.Feb 17 2021, 6:53 PM

sys/ddb/db_input.c
449	Hmm. No I didn't think much about that. Unfortunately I think we may see odd stuff either way. If the buffer is flushed, then as input continues to be received it will be buffered, so for illustration if the buffer size were 4 and this were received x/s version we'd end up with `ion` in the buffer. On the other hand, the way it is currently coded can also result in a weird thing where `x/s` is buffered, then some number of characters are dropped as they are received, then if we are still receiving input as the buffer is drained characters are alternately dropped and buffered. I'm not sure what the least surprising behavior would be. Maybe it would be to drop all input until the buffer is completely drained and then to drain the console too? If you don't have a strong feeling about this, I might like to just go ahead with the current behavior and see if it's a problem in practice before doing anything elaborate.

markj feedback: man style

Harbormaster completed remote builds in B37106: Diff 84120.Feb 17 2021, 7:09 PM

markj accepted this revision.Feb 17 2021, 7:24 PM

markj added inline comments.

sys/ddb/db_input.c
449	I'm ok with committing the change as-is. I don't have any strong feelings about this either, I don't think I rely much on typeahead in ddb in any case.

This revision is now accepted and ready to land.Feb 17 2021, 7:24 PM

Closed by commit rG3e5e9939cda3: ddb: enable the use of ^C and ^S/^Q (authored by rlibby). · Explain WhyMar 14 2021, 11:20 PM

This revision was automatically updated to reflect the committed changes.

rlibby added a commit: rG3e5e9939cda3: ddb: enable the use of ^C and ^S/^Q.

ddb: enable the use of ^C and ^S/^Q
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 85749

share/man/man4/ddb.4

sys/ddb/db_input.c

sys/ddb/db_output.c

sys/ddb/ddb.h

ddb: enable the use of ^C and ^S/^QClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 85749

share/man/man4/ddb.4

sys/ddb/db_input.c

sys/ddb/db_output.c

sys/ddb/ddb.h

ddb: enable the use of ^C and ^S/^Q
ClosedPublic
Actions

Revision Contents
Changeset List