gdb(4): Don't escape GDB special characters at application layer
ClosedPublic
Actions

Authored by cem on Sep 30 2020, 12:11 AM.

Details

Reviewers

markj
jhb
khng
rlibby
emaste

Commits

rS366291: gdb(4): Don't escape GDB special characters at application layer

Summary

In r351368, we introduced this XML- and GDB-encoded data. The protocol
'offset' should reflex the logical XML data offset, but unfortunately we
counted the GDB escapes as well.

In fact, we cannot safely do GDB character escaping at this layer at
all, because we don't know what will be flushed in a packet. It is
bogus to send only the first character of a two-character escape
sequence.

This patch "corrects" the problem by squashing these characters in the
transmitted XML document. It would be nice to transmit the characters
faithfully, but that is a more complicated change. Thread names are a
nice convenience feature for the GDB client, but one can always inspect
td_name or p_comm directly to find the true name.

Reported by: Ka Ho Ng <khng300 AT gmail.com>

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cem created this revision.Sep 30 2020, 12:11 AM

Herald added a subscriber: emaste. · View Herald TranscriptSep 30 2020, 12:11 AM

cem requested review of this revision.Sep 30 2020, 12:11 AM

Harbormaster completed remote builds in B33893: Diff 77664.Sep 30 2020, 12:11 AM

cem added a subscriber: rlibby.Sep 30 2020, 12:16 AM

Tested. So far it runs well on my setup.

This revision is now accepted and ready to land.Sep 30 2020, 2:22 AM

Seems fine, especially as we already squash other characters instead of
escaping them.

It feels like we should be able to handle this in qXfer_drain by doing
the gdb escape there and then just not draining the whole qxfer buffer
if we run out of space in the tx buffer. However, I haven't looked
around enough to know whether that'd actually work.

There's definitely some way to make it work, it just adds complexity and IIRC may require limiting our typical (no escaped characters) packet size to half of our advertised max packet size (in case the entire packet needs escaping). Or fragmenting any packets that go over into a normal packet and a tiny extra packet (this seems worse than just using half advertised packet size).

Seems reasonable to me.

Closed by commit rS366291: gdb(4): Don't escape GDB special characters at application layer (authored by cem). · Explain WhySep 30 2020, 2:56 PM

This revision was automatically updated to reflect the committed changes.

cem added a commit: rS366291: gdb(4): Don't escape GDB special characters at application layer.

Herald added a subscriber: imp. · View Herald TranscriptSep 30 2020, 2:56 PM

Revision Contents
Changeset List

Path

Size

head/

sys/

gdb/

gdb_main.c

14 lines

Diff 77682

View Options

head/sys/gdb/gdb_main.c

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines	#endif
* fall back to non RFC1323 RTT calculation. Normalize		* fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection		* timestamp if syncookies were used when this connection
* was established.		* was established.
*/		*/
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {		if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset;		to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {		if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0;		to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
}		}
}		}
/*		/*
* Process options only when we get SYN/ACK back. The SYN case		* Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache.		* for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN>		* According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled.		* or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up.		* XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines	if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 &&		(to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) {		TAILQ_EMPTY(&tp->snd_holes)) {
/*		/*
* This is a pure ack for outstanding data.		* This is a pure ack for outstanding data.
*/		*/
TCPSTAT_INC(tcps_predack);		TCPSTAT_INC(tcps_predack);

/*		/*
* "bad retransmit" recovery without timestamps.		* "bad retransmit" recovery.
*/		*/
if ((to.to_flags & TOF_TS) == 0 &&		if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&		tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&		tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) {		(((to.to_flags & TOF_TS) != 0 &&
		to.to_tsecr != 0 &&
		TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) \|\|
		((to.to_flags & TOF_TS) == 0 &&
		TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR);		cc_cong_signal(tp, th, CC_RTO_ERR);
}

/*		/*
* Recalculate the transmit timer / rtt.		* Recalculate the transmit timer / rtt.
*		*
* Some boxes send broken timestamp replies		* Some boxes send broken timestamp replies
		rscheffUnsubmitted Done Inline Actions Any specific reason to write the or statement as two separate if clauses? I believe in other parts we do it ((true & x & y & z) \| (false & a & b &c )) - as multiline checks; I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good. The logic looks ok to me. rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
		glebiusUnsubmitted Done Inline Actions I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading. Oh, the upper level embracing if can also be merged into one big clause: if ((tp->t_rxtshift == 1 && tp->t_flags & TF_PREVVALID && tp->t_badrxtwin != 0) && (((to.to_flags & TOF_TS) != 0 && (to.to_tsecr != 0) && TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) \|\| ((to.to_flags & TOF_TS) == 0 && TSTMP_LT(ticks, tp->t_badrxtwin)))) cc_cong_signal(tp, th, CC_RTO_ERR); This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b \|\| c)). glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
		tuexenAuthorUnsubmitted Done Inline Actions Now using one and two spaces of indentation to make the expression more readable. tuexen: Now using one and two spaces of indentation to make the expression more readable.
		tuexenAuthorUnsubmitted Done Inline Actions It was a try to break things up for improved readability by humans. tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore		* during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a		* timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer.		* huge RTT and blow up the retransmit timer.
*/		*/
if ((to.to_flags & TOF_TS) != 0 &&		if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) {		to.to_tsecr) {
uint32_t t;		uint32_t t;

▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines

gdb(4): Don't escape GDB special characters at application layerClosedPublicActions