Commit 0b45cab1 authored by Andrea Shepard's avatar Andrea Shepard
Browse files

Merge branch 'bug18570_027' into maint-0.2.7

parents 1218d731 72ebf416

changes/bug18570

0 → 100644
+7 −0
Original line number Diff line number Diff line 
  o Minor bugfixes (correctness):

    - Fix a bad memory handling bug that would occur if we had queued

      a cell on a channel's incoming queue. Fortunately, we can't actually

      queue a cell like that as our code is constructed today, but it's best

      to avoid this kind of error, even if there isn't any code that triggers

      it today. Fixes bug 18570; bugfix on 0.2.4.4-alpha.

src/or/channel.c

+34 −4
Original line number Diff line number Diff line
@@ -2652,6 +2652,11 @@ channel_process_cells(channel_t *chan) 
  /*

   * Process cells until we're done or find one we have no current handler

   * for.

   *

   * We must free the cells here after calling the handler, since custody

   * of the buffer was given to the channel layer when they were queued;

   * see comments on memory management in channel_queue_cell() and in

   * channel_queue_var_cell() below.

   */

  while (NULL != (q = TOR_SIMPLEQ_FIRST(&chan->incoming_queue))) {

    tor_assert(q);
@@ -2669,6 +2674,7 @@ channel_process_cells(channel_t *chan) 
                q->u.fixed.cell, chan,

                U64_PRINTF_ARG(chan->global_identifier));

      chan->cell_handler(chan, q->u.fixed.cell);

      tor_free(q->u.fixed.cell);

      tor_free(q);

    } else if (q->type == CELL_QUEUE_VAR &&

               chan->var_cell_handler) {
@@ -2681,6 +2687,7 @@ channel_process_cells(channel_t *chan) 
                q->u.var.var_cell, chan,

                U64_PRINTF_ARG(chan->global_identifier));

      chan->var_cell_handler(chan, q->u.var.var_cell);

      tor_free(q->u.var.var_cell);

      tor_free(q);

    } else {

      /* Can't handle this one */
@@ -2701,6 +2708,7 @@ channel_queue_cell(channel_t *chan, cell_t *cell) 
{

  int need_to_queue = 0;

  cell_queue_entry_t *q;

  cell_t *cell_copy = NULL;



  tor_assert(chan);

  tor_assert(cell);
@@ -2728,8 +2736,19 @@ channel_queue_cell(channel_t *chan, cell_t *cell) 
              U64_PRINTF_ARG(chan->global_identifier));

    chan->cell_handler(chan, cell);

  } else {

    /* Otherwise queue it and then process the queue if possible. */

    q = cell_queue_entry_new_fixed(cell);

    /*

     * Otherwise queue it and then process the queue if possible.

     *

     * We queue a copy, not the original pointer - it might have been on the

     * stack in connection_or_process_cells_from_inbuf() (or another caller

     * if we ever have a subclass other than channel_tls_t), or be freed

     * there after we return.  This is the uncommon case; the non-copying

     * fast path occurs in the if (!need_to_queue) case above when the

     * upper layer has installed cell handlers.

     */

    cell_copy = tor_malloc_zero(sizeof(cell_t));

    memcpy(cell_copy, cell, sizeof(cell_t));

    q = cell_queue_entry_new_fixed(cell_copy);

    log_debug(LD_CHANNEL,

              "Queueing incoming cell_t %p for channel %p "

              "(global ID " U64_FORMAT ")",
@@ -2755,6 +2774,7 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) 
{

  int need_to_queue = 0;

  cell_queue_entry_t *q;

  var_cell_t *cell_copy = NULL;



  tor_assert(chan);

  tor_assert(var_cell);
@@ -2783,8 +2803,18 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) 
              U64_PRINTF_ARG(chan->global_identifier));

    chan->var_cell_handler(chan, var_cell);

  } else {

    /* Otherwise queue it and then process the queue if possible. */

    q = cell_queue_entry_new_var(var_cell);

    /*

     * Otherwise queue it and then process the queue if possible.

     *

     * We queue a copy, not the original pointer - it might have been on the

     * stack in connection_or_process_cells_from_inbuf() (or another caller

     * if we ever have a subclass other than channel_tls_t), or be freed

     * there after we return.  This is the uncommon case; the non-copying

     * fast path occurs in the if (!need_to_queue) case above when the

     * upper layer has installed cell handlers.

     */

    cell_copy = var_cell_copy(var_cell);

    q = cell_queue_entry_new_var(cell_copy);

    log_debug(LD_CHANNEL,

              "Queueing incoming var_cell_t %p for channel %p "

              "(global ID " U64_FORMAT ")",

src/or/channeltls.c

+11 −0
Original line number Diff line number Diff line
@@ -1009,6 +1009,11 @@ channel_tls_time_process_cell(cell_t *cell, channel_tls_t *chan, int *time, 
 * for cell types specific to the handshake for this transport protocol and

 * handles them, and queues all other cells to the channel_t layer, which

 * eventually will hand them off to command.c.

 *

 * The channel layer itself decides whether the cell should be queued or

 * can be handed off immediately to the upper-layer code.  It is responsible

 * for copying in the case that it queues; we merely pass pointers through

 * which we get from connection_or_process_cells_from_inbuf().

 */



void
@@ -1106,6 +1111,12 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) 
 * related and live below the channel_t layer, so no variable-length

 * cells ever get delivered in the current implementation, but I've left

 * the mechanism in place for future use.

 *

 * If we were handing them off to the upper layer, the channel_t queueing

 * code would be responsible for memory management, and we'd just be passing

 * pointers through from connection_or_process_cells_from_inbuf().  That

 * caller always frees them after this function returns, so this function

 * should never free var_cell.

 */



void

src/or/connection_or.c

+35 −0
Original line number Diff line number Diff line
@@ -488,6 +488,28 @@ var_cell_new(uint16_t payload_len) 
  return cell;

}



/**

 * Copy a var_cell_t

 */



var_cell_t *

var_cell_copy(const var_cell_t *src)

{

  var_cell_t *copy = NULL;

  size_t size = 0;



  if (src != NULL) {

    size = STRUCT_OFFSET(var_cell_t, payload) + src->payload_len;

    copy = tor_malloc_zero(size);

    copy->payload_len = src->payload_len;

    copy->command = src->command;

    copy->circ_id = src->circ_id;

    memcpy(copy->payload, src->payload, copy->payload_len);

  }



  return copy;

}



/** Release all space held by <b>cell</b>. */

void

var_cell_free(var_cell_t *cell)
@@ -2060,6 +2082,19 @@ connection_or_process_cells_from_inbuf(or_connection_t *conn) 
{

  var_cell_t *var_cell;



  /*

   * Note on memory management for incoming cells: below the channel layer,

   * we shouldn't need to consider its internal queueing/copying logic.  It

   * is safe to pass cells to it on the stack or on the heap, but in the

   * latter case we must be sure we free them later.

   *

   * The incoming cell queue code in channel.c will (in the common case)

   * decide it can pass them to the upper layer immediately, in which case

   * those functions may run directly on the cell pointers we pass here, or

   * it may decide to queue them, in which case it will allocate its own

   * buffer and copy the cell.

   */



  while (1) {

    log_debug(LD_OR,

              TOR_SOCKET_T_FORMAT": starting, inbuf_datalen %d "

src/or/connection_or.h

+1 −0
Original line number Diff line number Diff line
@@ -97,6 +97,7 @@ void cell_pack(packed_cell_t *dest, const cell_t *src, int wide_circ_ids); 
int var_cell_pack_header(const var_cell_t *cell, char *hdr_out,

                         int wide_circ_ids);

var_cell_t *var_cell_new(uint16_t payload_len);

var_cell_t *var_cell_copy(const var_cell_t *src);

void var_cell_free(var_cell_t *cell);



/** DOCDOC */