Opened 9 years ago

Closed 3 years ago

#3428 closed defect (fixed)

Control port may emit log messages in the middle of another event/reply

Reported by: rransom Owned by: nickm
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

If the following call to connection_printf_to_buf in handle_control_getinfo in src/or/control.c fails, it may emit a LOG control-port event in the middle of a reply:

    if (!strchr(v, '\n') && !strchr(v, '\r')) {
      connection_printf_to_buf(conn, "250-%s=", k);
      connection_write_str_to_buf(v, conn);
      connection_write_str_to_buf("\r\n", conn);
    } else {

I expect that other output functions can emit log messages in the middle of a control port event or reply, too. We should make sure that never happens, by making all control-port code build each complete reply/event in a separate buffer before writing any of it out, and adding an event/reply queue to the control_connection_t structure.

Child Tickets

Change History (7)

comment:1 Changed 9 years ago by rransom

Milestone: Tor: 0.2.3.x-finalTor: unspecified
Status: newneeds_review

See bug3428b ( git://git.torproject.org/rransom/tor.git bug3428b ) for a patch for master to make connection_printf_to_buf exit (rather than warn) if tor_vasprintf fails. I expect that there are other cases where the control-port code can emit a log message in the middle of another event or reply, so this patch doesn't resolve the ticket.

See bug3428 ( git://git.torproject.org/rransom/tor.git bug3428 ) for an implementation of a linked-list-based queue that should be useful when we do really fix this bug. (It needs more unit tests first.)

I don't know what to put into the queue -- buf_ts? evbuffers? smartlist_ts of sized_chunk_ts? Whatever we use needs to have a printf_to_X function and a copy_X_to_bufferevent function, and be at least tolerably lightweight.

comment:2 Changed 8 years ago by nickm

For 3428b, I feel okay merging it into 0.2.3, but adding a new assert() to 0.2.2 would make me nervous. Does that sound plausible to you, rransom? For less fatal cases than this one, I'd prefer a situation where we add LOG_NOCB to the message rather than simply not emitting it.

For queue stuff in bug3428, I kinda like the sys/queue.h stuff that the BSDs have. Is there a reason not to snarf that?

Actually, perhaps instead of a generic queue like this, we could just add an "move the contents of buf_t X onto the end of buf_t Y" interface to match the behavior of evbuffer_add_buffer(). Then we wouldn't need to have a separate queue: we could use a generic_buffer_t as the queue. Thoughts?

comment:3 in reply to:  2 Changed 8 years ago by rransom

Replying to nickm:

For 3428b, I feel okay merging it into 0.2.3, but adding a new assert() to 0.2.2 would make me nervous. Does that sound plausible to you, rransom? For less fatal cases than this one, I'd prefer a situation where we add LOG_NOCB to the message rather than simply not emitting it.

I agree.

For queue stuff in bug3428, I kinda like the sys/queue.h stuff that the BSDs have. Is there a reason not to snarf that?

Actually, perhaps instead of a generic queue like this, we could just add an "move the contents of buf_t X onto the end of buf_t Y" interface to match the behavior of evbuffer_add_buffer(). Then we wouldn't need to have a separate queue: we could use a generic_buffer_t as the queue. Thoughts?

That sounds better.

comment:4 Changed 8 years ago by nickm

Owner: changed from rransom to nickm
Status: needs_reviewaccepted

Okay; merged your bug3428b, and throwing this back into "accepted".

comment:5 Changed 7 years ago by nickm

Keywords: tor-client added

comment:6 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:7 Changed 3 years ago by nickm

Resolution: fixed
Severity: Normal
Status: acceptedclosed

All remaining cases of this should have been fixed when we merged #16695 . Even if generating an event generates an event, the events should still get serialized.

Note: See TracTickets for help on using tickets.