Skip to content

Commit

Permalink
peer_control: Fix a use-after-free bug. (ElementsProject#1237)
Browse files Browse the repository at this point in the history
This bug is a classic case of being lazy:
1. peer_accept_channel() allocated its return off the input message,
   rather than taking an explicit allocation context.  This concealed the
   lifetime nature of the return.
2. The context for sanitize_error was the error itself, rather than the
   more obvious tmpctx (connect_failed does not take).

The global tmpctx removes the "efficiency" excuse for grabbing a random
object to use as context, and is also nice and explicit.

All-the-hard-work-by: @ZmnSCPxj
  • Loading branch information
ZmnSCPxj authored and rustyrussell committed Mar 19, 2018
1 parent e56eee5 commit 044705a
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 12 deletions.
5 changes: 3 additions & 2 deletions lightningd/opening_control.c
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,8 @@ static void channel_config(struct lightningd *ld,
* NULL if we took over, otherwise hand back to gossipd with this
* error.
*/
u8 *peer_accept_channel(struct lightningd *ld,
u8 *peer_accept_channel(const tal_t *ctx,
struct lightningd *ld,
const struct pubkey *peer_id,
const struct wireaddr *addr,
const struct crypto_state *cs,
Expand All @@ -637,7 +638,7 @@ u8 *peer_accept_channel(struct lightningd *ld,
/* Fails if there's already one */
uc = new_uncommitted_channel(ld, NULL, peer_id, addr);
if (!uc)
return towire_errorfmt(open_msg, channel_id,
return towire_errorfmt(ctx, channel_id,
"Multiple channels unsupported");

uc->openingd = new_channel_subd(ld, "lightning_openingd", uc, uc->log,
Expand Down
5 changes: 3 additions & 2 deletions lightningd/opening_control.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ void json_add_uncommitted_channel(struct json_result *response,

/* Peer has spontaneously exited from gossip due to open msg. Return
* NULL if we took over, otherwise hand back to gossipd with this
* error.
* error (allocated off @ctx).
*/
u8 *peer_accept_channel(struct lightningd *ld,
u8 *peer_accept_channel(const tal_t *ctx,
struct lightningd *ld,
const struct pubkey *peer_id,
const struct wireaddr *addr,
const struct crypto_state *cs,
Expand Down
8 changes: 4 additions & 4 deletions lightningd/peer_control.c
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,8 @@ void peer_sent_nongossip(struct lightningd *ld,

/* Open request? */
if (fromwire_peektype(in_msg) == WIRE_OPEN_CHANNEL) {
error = peer_accept_channel(ld, id, addr, cs, gossip_index,
error = peer_accept_channel(tmpctx,
ld, id, addr, cs, gossip_index,
gfeatures, lfeatures,
peer_fd, gossip_fd, channel_id,
in_msg);
Expand All @@ -459,18 +460,17 @@ void peer_sent_nongossip(struct lightningd *ld,
}

/* Weird request. */
error = towire_errorfmt(ld, channel_id,
error = towire_errorfmt(tmpctx, channel_id,
"Unexpected message %i for peer",
fromwire_peektype(in_msg));

send_error:
/* Hand back to gossipd, with an error packet. */
connect_failed(ld, id, sanitize_error(error, error, NULL));
connect_failed(ld, id, sanitize_error(tmpctx, error, NULL));
msg = towire_gossipctl_hand_back_peer(ld, id, cs, gossip_index, error);
subd_send_msg(ld->gossip, take(msg));
subd_send_fd(ld->gossip, peer_fd);
subd_send_fd(ld->gossip, gossip_fd);
tal_free(error);
}

static enum watch_result funding_announce_cb(struct channel *channel,
Expand Down
6 changes: 2 additions & 4 deletions wallet/test/run-wallet.c
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ bool outpointfilter_matches(struct outpointfilter *of UNNEEDED,
struct outpointfilter *outpointfilter_new(tal_t *ctx UNNEEDED)
{ fprintf(stderr, "outpointfilter_new called!\n"); abort(); }
/* Generated stub for peer_accept_channel */
u8 *peer_accept_channel(struct lightningd *ld UNNEEDED,
u8 *peer_accept_channel(const tal_t *ctx UNNEEDED,
struct lightningd *ld UNNEEDED,
const struct pubkey *peer_id UNNEEDED,
const struct wireaddr *addr UNNEEDED,
const struct crypto_state *cs UNNEEDED,
Expand Down Expand Up @@ -333,9 +334,6 @@ u8 *towire_channel_funding_locked(const tal_t *ctx UNNEEDED, const struct short_
/* Generated stub for towire_channel_send_shutdown */
u8 *towire_channel_send_shutdown(const tal_t *ctx UNNEEDED)
{ fprintf(stderr, "towire_channel_send_shutdown called!\n"); abort(); }
/* Generated stub for towire_error */
u8 *towire_error(const tal_t *ctx UNNEEDED, const struct channel_id *channel_id UNNEEDED, const u8 *data UNNEEDED)
{ fprintf(stderr, "towire_error called!\n"); abort(); }
/* Generated stub for towire_errorfmt */
u8 *towire_errorfmt(const tal_t *ctx UNNEEDED,
const struct channel_id *channel UNNEEDED,
Expand Down

0 comments on commit 044705a

Please sign in to comment.