explicitly setting rte->next = null
Hi People I've been trying to find a segfault in a custom protocol I've been writing. I've finally tracked it down to a situation where bgp protocol, in bgp_rte_update, is calling rte_get_temp but not setting -> next to null, leaving it at whatever was left in RAM when the memory was allocated. Later, I see it's not null and try to access the ->next and eventually trigger a segfault. I've looked at the code and I can't see any reason why ->next shouldn't be set to null. So, does anyone know if this is going to cause problems later? The attached patch implements my change. I wonder if it might be better to explicitly set the rte pointed to by rte_get_temp to zero before using and returning it. I didn't want to make that change in case there are really good reasons why not. Cheers Nic C-L
On Wed, Aug 20, 2014 at 05:44:28PM +1200, Nic Cave-Lynch wrote:
Hi People
I've been trying to find a segfault in a custom protocol I've been writing. I've finally tracked it down to a situation where bgp protocol, in bgp_rte_update, is calling rte_get_temp but not setting -> next to null, leaving it at whatever was left in RAM when the memory was allocated. Later, I see it's not null and try to access the ->next and eventually trigger a segfault.
Generally, rte->next should be set by rte_recalculate() when it is inserted to the routing table and should not be accessed before that. If this causes crash to you, it is probably some other problem in the update path. What are the circumstances when your code accessed rte->next value? It was called regularly from rte_announce()? -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Hi Ondrej I'm trying to add an attribute on a bgp-advertised route in response to some event on my own protocol. When I search for the rte of the route, so that I can add the attribute, sometimes ->next is set, so I then look at that one to to add the attribute there too. If I don't set ->next = NULL in bgp, then there are times when it is invalid, purely as a result of garbage in memory, so I end up writing to random places and getting a segfault. Or am I approaching the whole thing in the wrong way? Thanks Nic C-L The update function is: void cst_update_route_from_msg( struct proto * p, ip_addr * prefix, int prefix_len, ip_addr * router_addr, bool had_added ) { eattr *ea; net * net; struct rte *rte; struct rte *new_rte; rta *temp_rta; rta *rta; net = net_find( p->table, *prefix, prefix_len); if (net) { rte = net->routes; temp_rta = lp_alloc(cst_linpool, sizeof(struct rta)); while (rte && rte->attrs) { memcpy(temp_rta, rte->attrs, sizeof(struct rta)); temp_rta->aflags &= ~RTAF_CACHED ; // say this is uncached rta = rta_lookup( temp_rta ); if (!rta) continue; new_rte = rte_get_temp(rta_clone(rta)); if (!new_rte) continue; new_rte->next = NULL; ea = ea_find(new_rte->attrs->eattrs, EA_CODE(EAP_BGP, BA_COMMUNITY)); if (ea) { u32 asn = cst_get_as_from_rte( rte ); int found_pub = cst_check_comm_attribs( rte->attrs->eattrs, (asn<<16)|PUB_VAL ); if (!found_pub) { ea->u.ptr = int_set_add(cst_linpool, d, (asn<<16)|PUB_VAL); // update the route table with the new attributes... new_rte->net = net; new_rte->pflags |= REF_FILTERED; // make this route not be ingored in rte_recalculate rte_update2(rte->attrs->src->proto->main_ahook, net, new_rte, rte->attrs->src); } } rte = rte->next; } } lp_flush(cst_linpool); } On Wed, 20 Aug 2014 19:37:38 +1200, Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Wed, Aug 20, 2014 at 05:44:28PM +1200, Nic Cave-Lynch wrote:
Hi People
I've been trying to find a segfault in a custom protocol I've been writing. I've finally tracked it down to a situation where bgp protocol, in bgp_rte_update, is calling rte_get_temp but not setting -> next to null, leaving it at whatever was left in RAM when the memory was allocated. Later, I see it's not null and try to access the ->next and eventually trigger a segfault.
Generally, rte->next should be set by rte_recalculate() when it is inserted to the routing table and should not be accessed before that. If this causes crash to you, it is probably some other problem in the update path. What are the circumstances when your code accessed rte->next value? It was called regularly from rte_announce()?
participants (2)
-
Nic Cave-Lynch -
Ondrej Zajicek