From e97f7c4e29ea221e01032f4c547301843bf92f41 Mon Sep 17 00:00:00 2001 From: Jeremy Lakeman Date: Mon, 22 Jun 2015 12:30:18 +0930 Subject: [PATCH] Fix vomp stun tests - Don't trust stun responses about 3rd parties - Only ack neighbour links via 1hop destinations (not multi-hop routes) - Don't override network destinations that were manually supplied --- network_cli.c | 10 ++++++---- overlay_link.c | 17 +++++++++++------ overlay_mdp.c | 2 ++ overlay_packet.h | 5 ++++- overlay_packetformats.c | 5 +++-- overlay_queue.c | 9 ++++++--- route_link.c | 16 ++++++++++------ tests/routing | 6 +++--- tests/vomp | 2 +- vomp.c | 4 ++-- 10 files changed, 48 insertions(+), 28 deletions(-) diff --git a/network_cli.c b/network_cli.c index 2aee8554..1b4a5ab9 100644 --- a/network_cli.c +++ b/network_cli.c @@ -324,14 +324,16 @@ static int app_trace(const struct cli_parsed *parsed, struct cli_context *contex int offset=0; { // skip the first two sid's - int len = mdp.out.payload[offset++]; - offset+=len; - len = mdp.out.payload[offset++]; + uint len = mdp.out.payload[offset++]; offset+=len; + if (offsetsource->sid)); if (header->source->reachable == REACHABLE_SELF) RETURN(0); @@ -165,7 +167,7 @@ overlay_mdp_service_probe(struct internal_mdp_header *header, struct overlay_buf addr.addrlen = ob_remaining(payload); if (addr.addrlen > sizeof(addr.store)) - RETURN(-1); + RETURN(WHY("Badly formatted probe packet")); ob_get_bytes(payload, (unsigned char*)&addr.addr, addr.addrlen); @@ -175,7 +177,7 @@ overlay_mdp_service_probe(struct internal_mdp_header *header, struct overlay_buf int overlay_send_probe(struct subscriber *peer, struct network_destination *destination, int queue){ time_ms_t now = gettime_ms(); - // though unicast probes don't typically use the same network destination, + // though unicast probes don't typically re-use the same network destination, // we should still try to throttle when we can if (destination->last_tx + destination->ifconfig.tick_ms > now) return WHY("Throttling probe packet"); @@ -295,10 +297,13 @@ int overlay_mdp_service_stun(struct internal_mdp_header *header, struct overlay_ if (!subscriber || (subscriber->reachable&REACHABLE_DIRECT)) continue; - struct network_destination *destination = create_unicast_destination(&addr, NULL); - if (destination){ - overlay_send_probe(subscriber, destination, OQ_MESH_MANAGEMENT); - release_destination_ref(destination); + // only trust stun responses from our directory service or about the packet sender. + if (directory_service == header->source || subscriber == header->source){ + struct network_destination *destination = create_unicast_destination(&addr, NULL); + if (destination){ + overlay_send_probe(subscriber, destination, OQ_MESH_MANAGEMENT); + release_destination_ref(destination); + } } } return 0; diff --git a/overlay_mdp.c b/overlay_mdp.c index c57baf5e..50a136ed 100644 --- a/overlay_mdp.c +++ b/overlay_mdp.c @@ -903,6 +903,8 @@ int _overlay_send_frame(struct __sourceloc whence, struct internal_mdp_header *h frame->queue = header->qos; frame->type = OF_TYPE_DATA; frame->resend = header->resend; + frame->send_context = header->send_context; + frame->send_hook = header->send_hook; if (!(header->crypt_flags & MDP_FLAG_NO_CRYPT)) frame->modifiers |= OF_CRYPTO_CIPHERED; diff --git a/overlay_packet.h b/overlay_packet.h index 5070157b..a5679622 100644 --- a/overlay_packet.h +++ b/overlay_packet.h @@ -61,13 +61,14 @@ struct overlay_frame { // callback and context just before packet sending void *send_context; - int (*send_hook)(struct overlay_frame *, int seq, void *context); + int (*send_hook)(struct overlay_frame *, struct network_destination *, int, void *); // when should we send it? time_ms_t delay_until; // where should we send it? struct packet_destination destinations[MAX_PACKET_DESTINATIONS]; int destination_count; + uint8_t manual_destinations; // how often have we sent it? int transmit_count; @@ -113,6 +114,8 @@ struct internal_mdp_header{ uint8_t qos; uint8_t crypt_flags; // combination of MDP_FLAG_NO_CRYPT & MDP_FLAG_NO_SIGN flags struct overlay_interface *receive_interface; + void *send_context; + int (*send_hook)(struct overlay_frame *, struct network_destination *, int, void *); }; diff --git a/overlay_packetformats.c b/overlay_packetformats.c index a4947f7f..243a739e 100644 --- a/overlay_packetformats.c +++ b/overlay_packetformats.c @@ -308,9 +308,10 @@ int parseEnvelopeHeader(struct decode_context *context, struct overlay_interface } if (config.debug.overlayframes) - DEBUGF("Received %s packet seq %d from %s on %s", + DEBUGF("Received %s packet seq %d from %s on %s %s", packet_flags & PACKET_UNICAST?"unicast":"broadcast", - sender_seq, alloca_tohex_sid_t(context->sender->sid), interface->name); + sender_seq, alloca_tohex_sid_t(context->sender->sid), + interface->name, alloca_socket_address(addr)); } link_received_packet(context, sender_seq, packet_flags & PACKET_UNICAST); diff --git a/overlay_queue.c b/overlay_queue.c index 14b23cf0..e2dbfa8c 100644 --- a/overlay_queue.c +++ b/overlay_queue.c @@ -197,6 +197,8 @@ int _overlay_payload_enqueue(struct __sourceloc __whence, struct overlay_frame * // allow the packet to be resent if (p->resend == 0) p->resend = 1; + }else{ + p->manual_destinations = 1; } int i=0; @@ -254,7 +256,7 @@ overlay_init_packet(struct outgoing_packet *packet, int packet_version, DEBUGF("Creating %d packet for interface %s, seq %d, %s", packet_version, destination->interface->name, destination->sequence_number, - destination->unicast?"unicast":"broadcast"); + alloca_socket_address(&destination->address)); return 0; } @@ -376,7 +378,8 @@ overlay_stuff_packet(struct outgoing_packet *packet, overlay_txqueue *queue, tim if (packet->buffer && ob_position(frame->payload) > ob_remaining(packet->buffer)) goto skip; - link_add_destinations(frame); + if (!frame->manual_destinations) + link_add_destinations(frame); if(frame->mdp_sequence != -1 && ((mdp_sequence - frame->mdp_sequence)&0xFFFF) >= 64){ // too late, we've sent too many packets for the next hop to correctly de-duplicate @@ -467,7 +470,7 @@ overlay_stuff_packet(struct outgoing_packet *packet, overlay_txqueue *queue, tim if (frame->send_hook){ // last minute check if we really want to send this frame, or track when we sent it - if (frame->send_hook(frame, packet->seq, frame->send_context)){ + if (frame->send_hook(frame, packet->destination, packet->seq, frame->send_context)){ // drop packet frame = overlay_queue_remove(queue, frame); continue; diff --git a/route_link.c b/route_link.c index f85f6b8b..9e2268ed 100644 --- a/route_link.c +++ b/route_link.c @@ -813,15 +813,18 @@ static int neighbour_find_best_link(struct neighbour *n) return 0; } -static int neighbour_link_sent(struct overlay_frame *UNUSED(frame), int sequence, void *context) +static int neighbour_link_sent(struct overlay_frame *frame, struct network_destination *destination, int sequence, void *context) { + frame->resend = -1; struct subscriber *subscriber = context; struct neighbour *neighbour = get_neighbour(subscriber, 0); if (!neighbour) return 0; neighbour->last_update_seq = sequence; if ((config.debug.linkstate && config.debug.verbose)||config.debug.ack) - DEBUGF("LINK STATE; ack sent to neighbour %s in seq %d", alloca_tohex_sid_t(subscriber->sid), sequence); + DEBUGF("LINK STATE; ack sent to neighbour %s via %s, in seq %d", + alloca_tohex_sid_t(subscriber->sid), + alloca_socket_address(&destination->address), sequence); return 0; } @@ -851,10 +854,11 @@ static int send_neighbour_link(struct neighbour *n) frame->send_context = n->subscriber; frame->resend = -1; - if (n->subscriber->reachable & REACHABLE){ + if (n->subscriber->reachable & REACHABLE_DIRECT){ + // let normal packet routing decisions pick the best link frame->destination = n->subscriber; }else{ - // no routing decision yet? send this packet to all probable destinations. + // not an immediate neighbour yet? send this packet to all probable destinations. if ((config.debug.linkstate && config.debug.verbose)|| config.debug.ack) DEBUGF("Sending link state ack to all possibilities"); struct link_out *out = n->out_links; @@ -1063,11 +1067,11 @@ int link_add_destinations(struct overlay_frame *frame) if (next_hop->reachable&REACHABLE_DIRECT){ unsigned i; - // do nothing if this packet is already going the right way - // or remove any stale destinations for (i=frame->destination_count;i>0;i--){ + // do nothing if this packet is already going the right way if (frame->destinations[i-1].destination == next_hop->destination) return 0; + // remove any stale destinations where the initial packet was not acked frame_remove_destination(frame, i-1); } frame_add_destination(frame, next_hop, next_hop->destination); diff --git a/tests/routing b/tests/routing index b75bade3..1c81f3d9 100755 --- a/tests/routing +++ b/tests/routing @@ -342,7 +342,7 @@ test_lowband_broadcast() { _simulator() { # TODO timeout & failure reporting? - executeOk --error-on-fail $servald_build_root/simulator <$SIM_IN + executeOk --timeout=120 --error-on-fail $servald_build_root/simulator <$SIM_IN tfw_cat --stdout --stderr rm $SIM_IN } @@ -855,7 +855,7 @@ test_unreliable_links() { received=$(sed -n -e 's/.*\<\([0-9]\+\) packets received.*/\1/p' "$TFWSTDOUT") || error "malformed ping output" duplicates=$(sed -n -e 's/.*\<\([0-9]\+\) duplicates.*/\1/p' "$TFWSTDOUT") || error "malformed ping output" assert [ "$received" -ge 20 ] - assert [ "$duplicates" -le 2 ] + assert [ "$duplicates" -le 10 ] # make sure the path is still there. wait_until path_exists +A +B +C wait_until path_exists +C +B +A @@ -901,7 +901,7 @@ test_unreliable_links2() { received=$(sed -n -e 's/.*\<\([0-9]\+\) packets received.*/\1/p' "$TFWSTDOUT") || error "malformed ping output" duplicates=$(sed -n -e 's/.*\<\([0-9]\+\) duplicates.*/\1/p' "$TFWSTDOUT") || error "malformed ping output" assert [ "$received" -ge 20 ] - assert [ "$duplicates" -le 2 ] + assert [ "$duplicates" -le 10 ] wait_until --timeout=20 path_exists +A +B +C +D wait_until --timeout=20 path_exists +D +C +B +A } diff --git a/tests/vomp b/tests/vomp index b7ed6566..07d3e7f7 100755 --- a/tests/vomp +++ b/tests/vomp @@ -209,7 +209,7 @@ setup_stun() { foreach_instance +A +B +C add_servald_interface --file 1 foreach_instance +A +B \ executeOk_servald config \ - set interfaces.1.drop_broadcasts on + set interfaces.1.broadcast.drop on start_servald_instances +A +B +C } has_unicast_link() { diff --git a/vomp.c b/vomp.c index 716569e1..42499e0e 100644 --- a/vomp.c +++ b/vomp.c @@ -727,8 +727,8 @@ static int vomp_process_audio(struct vomp_call_state *call, struct overlay_buffe uint32_t decoded_time = to_absolute_value(time, call->remote_audio_clock); uint32_t decoded_sequence = to_absolute_value(sequence, call->remote.sequence); - if (call->remote_audio_clock < decoded_time && - call->remote.sequence < decoded_sequence){ + if (call->remote_audio_clock <= decoded_time && + call->remote.sequence <= decoded_sequence){ call->remote_audio_clock = decoded_time; call->remote.sequence = decoded_sequence; }else if (call->remote_audio_clock < decoded_time ||