From b62ea34afed7d3bf60a6c8ef5a030fea52f55b10 Mon Sep 17 00:00:00 2001 From: Natanael Copa Date: Fri, 6 Jan 2017 16:18:45 +0100 Subject: [PATCH] ntpd: improve postponed hostname resolution Run the namelookup from the main loop so a misspelled first ntp server name does not block everything forever. This fixes the following situation which would block forever: $ sudo ./busybox ntpd -dn -p foobar -p pool.ntp.org ntpd: bad address 'foobar' ntpd: bad address 'foobar' ntpd: bad address 'foobar' ... New behavior: ntpd: bad address 'foobar' ntpd: sending query to 137.190.2.4 ntpd: reply from 137.190.2.4: offset:-1.009775 delay:0.175550 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x01 ntpd: sending query to 137.190.2.4 ntpd: reply from 137.190.2.4: offset:-1.009605 delay:0.175461 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x03 ntpd: sending query to 137.190.2.4 ntpd: reply from 137.190.2.4: offset:-1.005327 delay:0.167027 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x07 ntpd: sending query to 137.190.2.4 ntpd: bad address 'foobar' ntpd: reply from 137.190.2.4: offset:-1.046349 delay:0.248705 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x0f This patch is based on Kaarle Ritvanens work. http://lists.busybox.net/pipermail/busybox/2016-May/084197.html function old new delta ntpd_main 1061 1079 +18 ntp_init 556 560 +4 resolve_peer_hostname 81 75 -6 ------------------------------------------------------------------------------ (add/remove: 0/0 grow/shrink: 2/1 up/down: 22/-6) Total: 16 bytes Signed-off-by: Kaarle Ritvanen Signed-off-by: Natanael Copa Signed-off-by: Denys Vlasenko --- networking/ntpd.c | 73 ++++++++++++++++++++++------------------------- 1 file changed, 34 insertions(+), 39 deletions(-) diff --git a/networking/ntpd.c b/networking/ntpd.c index b7fa5dce9..bfd5705fc 100644 --- a/networking/ntpd.c +++ b/networking/ntpd.c @@ -155,6 +155,7 @@ #define RETRY_INTERVAL 32 /* on send/recv error, retry in N secs (need to be power of 2) */ #define NOREPLY_INTERVAL 512 /* sent, but got no reply: cap next query by this many seconds */ #define RESPONSE_INTERVAL 16 /* wait for reply up to N secs */ +#define HOSTNAME_INTERVAL 5 /* hostname lookup failed. Wait N secs for next try */ /* Step threshold (sec). std ntpd uses 0.128. */ @@ -790,28 +791,20 @@ reset_peer_stats(peer_t *p, double offset) VERB6 bb_error_msg("%s->lastpkt_recv_time=%f", p->p_dotted, p->lastpkt_recv_time); } -static void -resolve_peer_hostname(peer_t *p, int loop_on_fail) +static len_and_sockaddr* +resolve_peer_hostname(peer_t *p) { - len_and_sockaddr *lsa; - - again: - lsa = host2sockaddr(p->p_hostname, 123); - if (!lsa) { - /* error message already emitted by host2sockaddr() */ - if (!loop_on_fail) - return; -//FIXME: do this to avoid infinite looping on typo in a hostname? -//well... in which case, what is a good value for loop_on_fail? - //if (--loop_on_fail == 0) - // xfunc_die(); - sleep(5); - goto again; + len_and_sockaddr *lsa = host2sockaddr(p->p_hostname, 123); + if (lsa) { + free(p->p_lsa); + free(p->p_dotted); + p->p_lsa = lsa; + p->p_dotted = xmalloc_sockaddr2dotted_noport(&lsa->u.sa); + } else { + /* error message is emitted by host2sockaddr() */ + set_next(p, HOSTNAME_INTERVAL); } - free(p->p_lsa); - free(p->p_dotted); - p->p_lsa = lsa; - p->p_dotted = xmalloc_sockaddr2dotted_noport(&lsa->u.sa); + return lsa; } static void @@ -822,28 +815,28 @@ add_peers(const char *s) p = xzalloc(sizeof(*p) + strlen(s)); strcpy(p->p_hostname, s); - resolve_peer_hostname(p, /*loop_on_fail=*/ 1); + p->p_fd = -1; + p->p_xmt_msg.m_status = MODE_CLIENT | (NTP_VERSION << 3); + p->next_action_time = G.cur_time; /* = set_next(p, 0); */ + reset_peer_stats(p, STEP_THRESHOLD); /* Names like N..pool.ntp.org are randomly resolved * to a pool of machines. Sometimes different N's resolve to the same IP. * It is not useful to have two peers with same IP. We skip duplicates. */ - for (item = G.ntp_peers; item != NULL; item = item->link) { - peer_t *pp = (peer_t *) item->data; - if (strcmp(p->p_dotted, pp->p_dotted) == 0) { - bb_error_msg("duplicate peer %s (%s)", s, p->p_dotted); - free(p->p_lsa); - free(p->p_dotted); - free(p); - return; + if (resolve_peer_hostname(p)) { + for (item = G.ntp_peers; item != NULL; item = item->link) { + peer_t *pp = (peer_t *) item->data; + if (pp->p_dotted && strcmp(p->p_dotted, pp->p_dotted) == 0) { + bb_error_msg("duplicate peer %s (%s)", s, p->p_dotted); + free(p->p_lsa); + free(p->p_dotted); + free(p); + return; + } } } - p->p_fd = -1; - p->p_xmt_msg.m_status = MODE_CLIENT | (NTP_VERSION << 3); - p->next_action_time = G.cur_time; /* = set_next(p, 0); */ - reset_peer_stats(p, STEP_THRESHOLD); - llist_add_to(&G.ntp_peers, p); G.peer_cnt++; } @@ -871,6 +864,11 @@ do_sendto(int fd, static void send_query_to_peer(peer_t *p) { + if (!p->p_lsa) { + if (!resolve_peer_hostname(p)) + return; + } + /* Why do we need to bind()? * See what happens when we don't bind: * @@ -2238,7 +2236,7 @@ static NOINLINE void ntp_init(char **argv) IF_FEATURE_NTPD_SERVER("I:") /* compat */ "d" /* compat */ "46aAbgL", /* compat, ignored */ - &peers,&G.script_name, + &peers, &G.script_name, #if ENABLE_FEATURE_NTPD_SERVER &G.if_name, #endif @@ -2263,9 +2261,6 @@ static NOINLINE void ntp_init(char **argv) if (opts & OPT_N) setpriority(PRIO_PROCESS, 0, -15); - /* add_peers() calls can retry DNS resolution (possibly forever). - * Daemonize before them, or else boot can stall forever. - */ if (!(opts & OPT_n)) { bb_daemonize_or_rexec(DAEMON_DEVNULL_STDIO, argv); logmode = LOGMODE_NONE; @@ -2400,7 +2395,7 @@ int ntpd_main(int argc UNUSED_PARAM, char **argv) /* What if don't see it because it changed its IP? */ if (p->reachable_bits == 0) - resolve_peer_hostname(p, /*loop_on_fail=*/ 0); + resolve_peer_hostname(p); set_next(p, timeout); }