From: Dave Hart Date: Sat, 25 Apr 2009 08:08:01 +0000 (+0000) Subject: * [Bug 1165] Clean up small memory leaks in the config file parser X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=feb075adff031bedb6caee8e1486a90be545bb62;p=thirdparty%2Fntp.git * [Bug 1165] Clean up small memory leaks in the config file parser * Correct logconfig keyword declaration to MULTIPLE_ARG * Enable filename and line number leak reporting on Windows when built DEBUG for all the typical C runtime allocators such as calloc, malloc, and strdup. Previously only emalloc calls were covered. * Add DEBUG-only code to free dynamically allocated memory that would otherwise remain allocated at ntpd exit, to allow less forgivable leaks to stand out in leaks reported after exit. * Ensure termination of strings in ports/winnt/libisc/isc_strerror.c and quiet compiler warnings. bk: 49f2c4e1z1pTHnIMK71LR2aV2lVOTw --- diff --git a/ChangeLog b/ChangeLog index 617aef4011..9b504e5df7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +* [Bug 1165] Clean up small memory leaks in the config file parser +* Correct logconfig keyword declaration to MULTIPLE_ARG +* Enable filename and line number leak reporting on Windows when built + DEBUG for all the typical C runtime allocators such as calloc, + malloc, and strdup. Previously only emalloc calls were covered. +* Add DEBUG-only code to free dynamically allocated memory that would + otherwise remain allocated at ntpd exit, to allow less forgivable + leaks to stand out in leaks reported after exit. +* Ensure termination of strings in ports/winnt/libisc/isc_strerror.c + and quiet compiler warnings. (4.2.5p165) 2009/04/23 Released by Harlan Stenn * WWVB refclock cleanup from Dave Mills. * Code cleanup: requested_key -> request_key. diff --git a/include/ntp_filegen.h b/include/ntp_filegen.h index a1514b3e50..90ccee1f31 100644 --- a/include/ntp_filegen.h +++ b/include/ntp_filegen.h @@ -49,3 +49,6 @@ extern void filegen_setup (FILEGEN *, u_long); extern void filegen_config (FILEGEN *, char *, u_int, u_int); extern FILEGEN *filegen_get (char *); extern void filegen_register (char *, const char *, FILEGEN *); +#ifdef DEBUG +extern FILEGEN *filegen_unregister(char *); +#endif diff --git a/include/ntp_stdlib.h b/include/ntp_stdlib.h index bfff4597ac..9496b5d6e4 100644 --- a/include/ntp_stdlib.h +++ b/include/ntp_stdlib.h @@ -44,11 +44,11 @@ extern int authusekey (keyid_t, int, const u_char *); extern u_long calyearstart (u_long); extern const char *clockname (int); extern int clocktime (int, int, int, int, int, u_long, u_long *, u_int32 *); -#if defined SYS_WINNT && defined DEBUG -# define emalloc(_c) debug_emalloc(_c, __FILE__, __LINE__) -extern void * debug_emalloc (u_int, char *, int); -#else +#if !defined(_MSC_VER) || !defined(_DEBUG) extern void * emalloc (u_int); +#else +#define emalloc(size) debug_emalloc(size, __FILE__, __LINE__) +extern void * debug_emalloc (u_int, char *, int); #endif extern int ntp_getopt (int, char **, const char *); extern void init_auth (void); diff --git a/include/ntpd.h b/include/ntpd.h index f8cfb18c89..1f616aee87 100644 --- a/include/ntpd.h +++ b/include/ntpd.h @@ -261,7 +261,7 @@ extern int config_priority_override; extern int config_priority; #endif -extern char const *ntp_signd_socket; +extern char *ntp_signd_socket; /* ntp_control.c */ extern int num_ctl_traps; diff --git a/libntp/emalloc.c b/libntp/emalloc.c index 2fe4c38621..f3a0905fdc 100644 --- a/libntp/emalloc.c +++ b/libntp/emalloc.c @@ -6,43 +6,42 @@ #include "ntp_syslog.h" #include "ntp_stdlib.h" -#if defined SYS_WINNT && defined DEBUG -#include -#endif - -#if defined SYS_WINNT && defined DEBUG +#if !defined(_MSC_VER) || !defined(_DEBUG) void * -debug_emalloc( - u_int size, - char *filename, - int line +emalloc( + u_int size ) { - char *mem; + void *mem = malloc(size); - if ((mem = (char *)_malloc_dbg(size, _NORMAL_BLOCK, filename, line)) == 0) { + if (!mem) { msyslog(LOG_ERR, "Exiting: No more memory!"); exit(1); } return mem; } -#else +#else /* below is _MSC_VER && _DEBUG */ +/* + * When using the debug MS CRT malloc, preserve the original caller's + * line and file via the emalloc macro. + */ void * -emalloc( - u_int size +debug_emalloc( + u_int size, + char *filename, + int line ) { - char *mem; + void *mem = _malloc_dbg(size, _NORMAL_BLOCK, filename, line); - if ((mem = (char *)malloc(size)) == 0) { + if (!mem) { msyslog(LOG_ERR, "Exiting: No more memory!"); exit(1); } return mem; } - -#endif +#endif /* _MSC_VER && _DEBUG */ diff --git a/libntp/msyslog.c b/libntp/msyslog.c index 5f26fc4102..d9f4a26b9b 100644 --- a/libntp/msyslog.c +++ b/libntp/msyslog.c @@ -141,15 +141,18 @@ void msyslog(int level, const char *fmt, ...) #endif va_list ap; char buf[1025], nfmt[256]; + int errval; /* * Save the error value as soon as possible */ + errval = errno; + #ifdef SYS_WINNT - int errval = GetLastError(); -#else - int errval = errno; -#endif + errval = GetLastError(); + if (NO_ERROR == errval) + errval = errno; +#endif /* SYS_WINNT */ #if defined(__STDC__) || defined(HAVE_STDARG_H) va_start(ap, fmt); diff --git a/ntpd/ntp_config.c b/ntpd/ntp_config.c index 0a01937e2a..3c4f6a4459 100644 --- a/ntpd/ntp_config.c +++ b/ntpd/ntp_config.c @@ -23,6 +23,7 @@ #include "ntp_refclock.h" #include "ntp_filegen.h" #include "ntp_stdlib.h" +#include "ntp_assert.h" #include "ntpsim.h" #include #include @@ -140,14 +141,21 @@ short default_ai_family = AF_UNSPEC; /* Default either IPv4 or IPv6 */ short default_ai_family = AF_INET; /* [Bug 891]: FIX ME */ #endif char *sys_phone[MAXPHONE] = {NULL}; /* ACTS phone numbers */ -char *keysdir = NTP_KEYSDIR; /* crypto keys directory */ +char default_keysdir[] = NTP_KEYSDIR; +char *keysdir = default_keysdir; /* crypto keys directory */ #if defined(HAVE_SCHED_SETSCHEDULER) int config_priority_override = 0; int config_priority; #endif const char *config_file; -const char *ntp_signd_socket; +char default_ntp_signd_socket[] = +#ifdef NTP_SIGND_PATH + NTP_SIGND_PATH; +#else + ""; +#endif +char *ntp_signd_socket; #ifdef HAVE_NETINFO struct netinfo_config_state *config_netinfo = NULL; int check_netinfo = 1; @@ -201,6 +209,10 @@ extern unsigned int qos; /* QoS setting */ static void call_proto_config_from_list(queue *flag_list, int able_flag); static void init_auth_node(void); static void init_syntax_tree(void); +#ifdef DEBUG +static void free_auth_node(void); + void free_syntax_tree(void); +#endif double *create_dval(double val); void destroy_restrict_node(struct restrict_node *my_node); static struct sockaddr_storage *get_next_address(struct address_node *addr); @@ -235,6 +247,14 @@ enum gnn_type { t_MSK /* Network Mask */ }; +#define DESTROY_QUEUE(q) \ +do { \ + if (q) { \ + destroy_queue(q); \ + (q) = NULL; \ + } \ +} while (0) + static unsigned long get_pfxmatch(char **s,struct masks *m); static unsigned long get_match(char *s,struct masks *m); static unsigned long get_logmask(char *s); @@ -276,16 +296,37 @@ init_auth_node(void) my_config.auth.crypto_cmd_list = NULL; my_config.auth.keys = NULL; my_config.auth.keysdir = NULL; -#ifdef NTP_SIGND_PATH - my_config.auth.ntp_signd_socket = NTP_SIGND_PATH; -#else - my_config.auth.ntp_signd_socket = NULL; -#endif + my_config.auth.ntp_signd_socket = default_ntp_signd_socket; my_config.auth.request_key = 0; my_config.auth.revoke = 0; my_config.auth.trusted_key_list = NULL; } +#ifdef DEBUG +static void +free_auth_node(void) +{ + DESTROY_QUEUE(my_config.auth.crypto_cmd_list); + + if (my_config.auth.keys) { + free(my_config.auth.keys); + my_config.auth.keys = NULL; + } + + if (my_config.auth.keysdir) { + free(my_config.auth.keysdir); + my_config.auth.keysdir = NULL; + } + + if (my_config.auth.ntp_signd_socket != default_ntp_signd_socket) { + free(my_config.auth.ntp_signd_socket); + my_config.auth.ntp_signd_socket = default_ntp_signd_socket; + } + + DESTROY_QUEUE(my_config.auth.trusted_key_list); +} +#endif /* DEBUG */ + static void init_syntax_tree(void) { @@ -316,8 +357,46 @@ init_syntax_tree(void) my_config.trap = create_queue(); my_config.vars = create_queue(); my_config.sim_details = NULL; + init_auth_node(); + +#ifdef DEBUG + atexit(free_syntax_tree); +#endif +} + +#ifdef DEBUG +void +free_syntax_tree(void) +{ + DESTROY_QUEUE(my_config.peers); + DESTROY_QUEUE(my_config.orphan_cmds); + + DESTROY_QUEUE(my_config.manycastserver); + DESTROY_QUEUE(my_config.multicastclient); + + DESTROY_QUEUE(my_config.stats_list); + DESTROY_QUEUE(my_config.filegen_opts); + + DESTROY_QUEUE(my_config.discard_opts); + DESTROY_QUEUE(my_config.restrict_opts); + + DESTROY_QUEUE(my_config.enable_opts); + DESTROY_QUEUE(my_config.disable_opts); + DESTROY_QUEUE(my_config.tinker); + DESTROY_QUEUE(my_config.fudge); + + DESTROY_QUEUE(my_config.logconfig); + DESTROY_QUEUE(my_config.phone); + DESTROY_QUEUE(my_config.qos); + DESTROY_QUEUE(my_config.setvar); + DESTROY_QUEUE(my_config.ttl); + DESTROY_QUEUE(my_config.trap); + DESTROY_QUEUE(my_config.vars); + + free_auth_node(); } +#endif /* DEBUG */ /* FUNCTIONS FOR CREATING NODES ON THE SYNTAX TREE * ----------------------------------------------- @@ -377,9 +456,9 @@ create_attr_sval( my_val = (struct attr_val *) get_node(sizeof(struct attr_val)); my_val->attr = attr; - if (s == NULL) - s = "\0"; /* free() and strdup() glow on NULL */ - my_val->value.s = strdup(s); + if (!s) /* free() hates NULL */ + s = strdup(""); + my_val->value.s = s; my_val->type = T_String; return my_val; } @@ -443,6 +522,8 @@ create_address_node( (struct address_node *) get_node(sizeof(struct address_node)); struct isc_netaddr temp_isc_netaddr; + NTP_REQUIRE(addr); + my_node->address = addr; if (type == 0) { if (is_ip_address(addr, &temp_isc_netaddr)) @@ -451,11 +532,25 @@ create_address_node( my_node->type = default_ai_family; } else { - my_node->type = type; + my_node->type = type; } return my_node; } + +void +destroy_address_node( + struct address_node *my_node + ) +{ + NTP_REQUIRE(my_node); + NTP_REQUIRE(my_node->address); + + free(my_node->address); + free_node(my_node); +} + + struct peer_node * create_peer_node( int hmode, @@ -532,8 +627,7 @@ create_peer_node( } free_node(my_val); } - if (options) - destroy_queue(options); + DESTROY_QUEUE(options); /* Check if errors were reported. If yes, ignore the node */ if (errflag) { @@ -587,11 +681,10 @@ destroy_restrict_node( * the restrict node */ if (my_node->addr) - free_node(my_node->addr); + destroy_address_node(my_node->addr); if (my_node->mask) - free_node(my_node->mask); - if (my_node->flags) - destroy_queue(my_node->flags); + destroy_address_node(my_node->mask); + DESTROY_QUEUE(my_node->flags); free_node(my_node); } @@ -689,7 +782,7 @@ create_sim_script_info( } free_node(my_attr_val); } - destroy_queue(script_queue); + DESTROY_QUEUE(script_queue); return (my_info); #endif } @@ -783,7 +876,7 @@ struct key_tok keyword_list[] = { { "fudge", T_Fudge, SINGLE_ARG }, { "includefile", T_Includefile, SINGLE_ARG }, { "leapfile", T_Leapfile, SINGLE_ARG }, - { "logconfig", T_Logconfig, SINGLE_ARG }, + { "logconfig", T_Logconfig, MULTIPLE_ARG }, { "logfile", T_Logfile, SINGLE_ARG }, { "manycastclient", T_Manycastclient, SINGLE_ARG }, { "manycastserver", T_Manycastserver, MULTIPLE_ARG }, @@ -964,8 +1057,7 @@ config_other_modes(void) if (getnetnum(addr_node->address, &addr_sock, 1, t_UNK) == 1) proto_config(PROTO_MULTICAST_ADD, 0, 0., &addr_sock); - free(addr_node->address); - free_node(addr_node); + destroy_address_node(addr_node); } sys_manycastserver = 1; } @@ -983,8 +1075,7 @@ config_other_modes(void) proto_config(PROTO_MULTICAST_ADD, 0, 0., &addr_sock); - free(addr_node->address); - free_node(addr_node); + destroy_address_node(addr_node); } proto_config(PROTO_MULTICAST_ADD, 1, 0., NULL); } @@ -998,27 +1089,30 @@ config_auth(void) int *key_val; /* Crypto Command */ - if (my_config.auth.crypto_cmd_list) { - while (!empty(my_config.auth.crypto_cmd_list)) { - my_val = (struct attr_val *) - dequeue(my_config.auth.crypto_cmd_list); + while (!empty(my_config.auth.crypto_cmd_list)) { + my_val = (struct attr_val *) + dequeue(my_config.auth.crypto_cmd_list); #ifdef OPENSSL - crypto_config(my_val->attr, my_val->value.s); + crypto_config(my_val->attr, my_val->value.s); #endif /* OPENSSL */ - free(my_val->value.s); - free_node(my_val); - } - destroy_queue(my_config.auth.crypto_cmd_list); - my_config.auth.crypto_cmd_list = NULL; + free(my_val->value.s); + free_node(my_val); } + DESTROY_QUEUE(my_config.auth.crypto_cmd_list); /* Keysdir Command */ - if (my_config.auth.keysdir) + if (my_config.auth.keysdir) { + if (keysdir != default_keysdir) + free(keysdir); keysdir = my_config.auth.keysdir; + } /* ntp_signd_socket Command */ - if (my_config.auth.ntp_signd_socket) + if (my_config.auth.ntp_signd_socket) { + if (ntp_signd_socket != default_ntp_signd_socket) + free(ntp_signd_socket); ntp_signd_socket = my_config.auth.ntp_signd_socket; + } #ifdef OPENSSL if (cryptosw) { @@ -1046,15 +1140,12 @@ config_auth(void) } /* Trusted Key Command */ - if (my_config.auth.trusted_key_list) { - while (!empty(my_config.auth.trusted_key_list)) { - key_val = (int *) dequeue(my_config.auth.trusted_key_list); - authtrust(*key_val, 1); - free_node(key_val); - } - destroy_queue(my_config.auth.trusted_key_list); - my_config.auth.trusted_key_list = NULL; + while (!empty(my_config.auth.trusted_key_list)) { + key_val = (int *) dequeue(my_config.auth.trusted_key_list); + authtrust(*key_val, 1); + free_node(key_val); } + DESTROY_QUEUE(my_config.auth.trusted_key_list); #ifdef OPENSSL /* Revoke Command */ @@ -1427,8 +1518,10 @@ config_setvar(void) while (!empty(my_config.setvar)) { my_node = (struct setvar_node *) dequeue(my_config.setvar); set_sys_var(my_node->data, my_node->len, my_node->def); + free(my_node->data); free_node(my_node); } + DESTROY_QUEUE(my_config.setvar); } static void @@ -1503,8 +1596,7 @@ config_trap(void) err_flag = 1; } - free(addr_node->address); - free_node(addr_node); + destroy_address_node(addr_node); } free_node(curr_opt); } @@ -1537,7 +1629,7 @@ config_trap(void) "can't set trap for %s, no resources", stoa(&peeraddr)); } - destroy_queue(curr_trap->options); + DESTROY_QUEUE(curr_trap->options); free_node(curr_trap); } } @@ -1632,7 +1724,7 @@ config_fudge(void) (struct refclockstat *)0); #endif - destroy_queue(curr_fudge->options); + DESTROY_QUEUE(curr_fudge->options); free_node(curr_fudge); } } @@ -1888,8 +1980,7 @@ config_peers(void) } /* Ok, everything done. Free up peer node memory */ - free(curr_peer->addr->address); - free_node(curr_peer->addr); + destroy_address_node(curr_peer->addr); free_node(curr_peer); } } @@ -1931,7 +2022,7 @@ config_sim(void) } free_node(init_stmt); } - destroy_queue(my_config.sim_details->init_opts); + DESTROY_QUEUE(my_config.sim_details->init_opts); /* Process the server list @@ -1953,7 +2044,7 @@ config_sim(void) free_node(serv_info); } } - destroy_queue(my_config.sim_details->servers); + DESTROY_QUEUE(my_config.sim_details->servers); /* Free the sim_node memory and set the sim_details as NULL */ free_node(my_config.sim_details); @@ -2016,11 +2107,12 @@ config_remotely(void) #if 0 init_syntax_tree(); #endif + key_scanner = create_keyword_scanner(keyword_list); yyparse(); -#ifdef DEBUG - if (debug > 1) - printf("Finished Parsing!!\n"); -#endif + delete_keyword_scanner(key_scanner); + key_scanner = NULL; + + DPRINTF(1, ("Finished Parsing!!\n")); config_ntpd(); @@ -2109,13 +2201,13 @@ getconfig( /*** BULK OF THE PARSER ***/ ip_file = fp[curr_include_level]; - key_scanner = create_keyword_scanner(keyword_list); init_syntax_tree(); + key_scanner = create_keyword_scanner(keyword_list); yyparse(); -#ifdef DEBUG - if (debug > 1) - printf("Finished Parsing!!\n"); -#endif + delete_keyword_scanner(key_scanner); + key_scanner = NULL; + + DPRINTF(1, ("Finished Parsing!!\n")); /* The actual configuration done depends on whether we are configuring the * simulator or the daemon. Perform a check and call the appropriate diff --git a/ntpd/ntp_data_structures.c b/ntpd/ntp_data_structures.c index 05c257cd2f..091ef1e7ec 100644 --- a/ntpd/ntp_data_structures.c +++ b/ntpd/ntp_data_structures.c @@ -59,20 +59,15 @@ void destroy_queue(queue *my_queue) void *get_node(size_t size) { - node *new_node; - new_node = (node *) emalloc(sizeof(node) + size); - if (new_node != NULL) { - new_node->node_next = NULL; - return new_node + 1; - } - else - return NULL; /* XXX: log this! */ + node *new_node = emalloc(sizeof(*new_node) + size); + new_node->node_next = NULL; + return new_node + 1; } /* Define a function to free the allocated memory for a queue node */ void free_node(void *my_node) { - node *old_node = (node *) my_node; + node *old_node = my_node; free(old_node - 1); } @@ -80,7 +75,7 @@ void free_node(void *my_node) /* Define a function to check if the queue is empty. */ int empty(queue *my_queue) { - return (my_queue->front == NULL); + return (!my_queue || !my_queue->front); } /* Define a function to add an element to the priority queue. diff --git a/ntpd/ntp_filegen.c b/ntpd/ntp_filegen.c index 411fb6c53b..1c0c431e19 100644 --- a/ntpd/ntp_filegen.c +++ b/ntpd/ntp_filegen.c @@ -553,18 +553,20 @@ filegen_register( return; } -#ifdef UNUSED -static FILEGEN * + +/* + * filegen_unregister frees memory allocated by filegen_register for + * name. + */ +#ifdef DEBUG +FILEGEN * filegen_unregister( char *name ) { struct filegen_entry **f = &filegen_registry; -#ifdef DEBUG - if (debug > 3) - printf("filegen_unregister(\"%s\")\n", name); -#endif + DPRINTF(3, ("filegen_unregister(\"%s\")\n", name)); while (*f) { if (strcmp((*f)->name,name) == 0) { diff --git a/ntpd/ntp_parser.y b/ntpd/ntp_parser.y index ceb0c7dd7b..3a04b4a299 100644 --- a/ntpd/ntp_parser.y +++ b/ntpd/ntp_parser.y @@ -760,12 +760,12 @@ miscellaneous_command ; drift_parm : T_String - { enqueue(my_config.vars, create_attr_sval(T_Driftfile, $1)); } - | T_String T_Double - { enqueue(my_config.vars, create_attr_dval(T_WanderThreshold, $2)); - enqueue(my_config.vars, create_attr_sval(T_Driftfile, $1)); } - | { /* Null driftfile, indicated by null string "\0" */ - enqueue(my_config.vars, create_attr_sval(T_Driftfile, "\0")); } + { enqueue(my_config.vars, create_attr_sval(T_Driftfile, $1)); } + | T_String T_Double + { enqueue(my_config.vars, create_attr_dval(T_WanderThreshold, $2)); + enqueue(my_config.vars, create_attr_sval(T_Driftfile, $1)); } + | /* Null driftfile, indicated by null string "\0" */ + { enqueue(my_config.vars, create_attr_sval(T_Driftfile, "\0")); } ; variable_assign @@ -796,12 +796,14 @@ log_config_command : T_String { char prefix = $1[0]; - char *type = &($1[1]); + char *type = $1 + 1; + if (prefix != '+' && prefix != '-' && prefix != '=') { yyerror("Logconfig prefix is not '+', '-' or '='\n"); } else - $$ = create_attr_sval(prefix, type); + $$ = create_attr_sval(prefix, strdup(type)); + YYFREE($1); } ; diff --git a/ntpd/ntp_scanner.c b/ntpd/ntp_scanner.c index 4652947e2e..a18d0ccabe 100644 --- a/ntpd/ntp_scanner.c +++ b/ntpd/ntp_scanner.c @@ -165,7 +165,7 @@ create_keyword_scanner( /* Define a function to delete the keyword scanner, freeing all the allocated * memory */ -static void +void delete_keyword_scanner( struct state *my_key_scanner ) diff --git a/ntpd/ntp_scanner.h b/ntpd/ntp_scanner.h index e5df1a5f74..0269402925 100644 --- a/ntpd/ntp_scanner.h +++ b/ntpd/ntp_scanner.h @@ -64,6 +64,7 @@ struct state *create_states(char *keyword, int expect_string, struct state *pre_state); struct state *create_keyword_scanner(struct key_tok *keyword_list); +void delete_keyword_scanner(struct state *my_key_scanner); int yylex(void); struct FILE_INFO *F_OPEN(const char *path, const char *mode); diff --git a/ntpd/ntp_util.c b/ntpd/ntp_util.c index 98a8843722..cf11521bca 100644 --- a/ntpd/ntp_util.c +++ b/ntpd/ntp_util.c @@ -110,8 +110,42 @@ static double prev_drift_comp; /* last frequency update */ static int leap_file(FILE *); static void record_sys_stats(void); + +/* + * uninit_util - free memory allocated by init_util + */ +#ifdef DEBUG +void +uninit_util(void) +{ + if (stats_drift_file) { + free(stats_drift_file); + free(stats_temp_file); + stats_drift_file = NULL; + stats_temp_file = NULL; + } + if (key_file_name) { + free(key_file_name); + key_file_name = NULL; + } + filegen_unregister("peerstats"); + filegen_unregister("loopstats"); + filegen_unregister("clockstats"); + filegen_unregister("rawstats"); + filegen_unregister("sysstats"); + filegen_unregister("protostats"); +#ifdef OPENSSL + filegen_unregister("cryptostats"); +#endif /* OPENSSL */ +#ifdef DEBUG_TIMING + filegen_unregister("timingstats"); +#endif /* DEBUG_TIMING */ +} +#endif /* DEBUG */ + + /* - * init_util - initialize the utilities + * init_util - initialize the utilities (ntpd included) */ void init_util(void) @@ -119,18 +153,21 @@ init_util(void) stats_drift_file = NULL; stats_temp_file = NULL; key_file_name = NULL; - filegen_register(&statsdir[0], "peerstats", &peerstats); - filegen_register(&statsdir[0], "loopstats", &loopstats); - filegen_register(&statsdir[0], "clockstats", &clockstats); - filegen_register(&statsdir[0], "rawstats", &rawstats); - filegen_register(&statsdir[0], "sysstats", &sysstats); - filegen_register(&statsdir[0], "protostats", &protostats); + filegen_register(statsdir, "peerstats", &peerstats); + filegen_register(statsdir, "loopstats", &loopstats); + filegen_register(statsdir, "clockstats", &clockstats); + filegen_register(statsdir, "rawstats", &rawstats); + filegen_register(statsdir, "sysstats", &sysstats); + filegen_register(statsdir, "protostats", &protostats); #ifdef OPENSSL - filegen_register(&statsdir[0], "cryptostats", &cryptostats); + filegen_register(statsdir, "cryptostats", &cryptostats); #endif /* OPENSSL */ #ifdef DEBUG_TIMING - filegen_register(&statsdir[0], "timingstats", &timingstats); + filegen_register(statsdir, "timingstats", &timingstats); #endif /* DEBUG_TIMING */ +#ifdef DEBUG + atexit(uninit_util); +#endif /* DEBUG */ } @@ -345,11 +382,11 @@ stats_config( * Open and read frequency file. */ case STATS_FREQ_FILE: - if (stats_drift_file != 0) { + if (stats_drift_file) { free(stats_drift_file); free(stats_temp_file); - stats_drift_file = 0; - stats_temp_file = 0; + stats_drift_file = NULL; + stats_temp_file = NULL; } if (value == 0 || (len = strlen(value)) == 0) diff --git a/ports/winnt/include/config.h b/ports/winnt/include/config.h index d951b79c8a..0094842de5 100644 --- a/ports/winnt/include/config.h +++ b/ports/winnt/include/config.h @@ -25,6 +25,16 @@ */ #define __STDC__ 1 +/* + * Enable the debug build of MS C runtime to dump leaks + * at exit time (currently only if run under a debugger). + */ +#if defined(_MSC_VER) && defined(_DEBUG) +#define _CRTDBG_MAP_ALLOC +#include +#include +#endif + /* * We need to include string.h first before we override strerror * otherwise we can get errors during the build @@ -90,7 +100,6 @@ #include #endif -/* #include */ /* must come after ws2tcpip.h */ #undef interface #include #include /* time_t for timeval decl */ diff --git a/ports/winnt/libisc/isc_strerror.c b/ports/winnt/libisc/isc_strerror.c index 55a72955b0..299d29165f 100644 --- a/ports/winnt/libisc/isc_strerror.c +++ b/ports/winnt/libisc/isc_strerror.c @@ -90,17 +90,41 @@ isc__strerror(int num, char *buf, size_t size) { initialize(); msg = isc__NTstrerror(num); - if (msg != NULL) - _snprintf(buf, size, "%s", msg); - else - _snprintf(buf, size, "Unknown error: %u", unum); + /* + * C4996 is strncpy, _snprintf may be unsafe, consider + * strncpy_s, _snprintf_s instead + * Those _s routines are not available with older MS + * compilers and our paranoid uses are safe. An + * alternative would be to use the _s versions for + * newer compilers instead of disabling warning 4996. + */ + #if defined(_MSC_VER) + #pragma warning(push) + #pragma warning(disable: 4996) + #endif /* _MSC_VER */ + + /* + * Both strncpy and _snprintf will not null terminate in the + * edge case of the message fitting but the terminator not. + * This seems to be a MS-specific behavior, but our paranoid + * approach should be harmless on less challenged runtimes. + */ + if (msg != NULL) { + strncpy(buf, msg, size - 1); + } else { + _snprintf(buf, size - 1, "Unknown error: %u", unum); + } + buf[size - 1] = '\0'; + + #if defined(_MSC_VER) + #pragma warning(pop) + #endif /* _MSC_VER */ } /* - * Note this will cause a memory leak unless the memory allocated here - * is freed by calling LocalFree. isc__strerror does this before unlocking. - * This only gets called if there is a system type of error and will likely - * be an unusual event. + * FormatError returns a message string for a given error code. + * Callers should not attempt to free the string, as it is cached + * and used again. */ char * FormatError(int error) { @@ -140,43 +164,122 @@ FormatError(int error) { 0, NULL); - if (lpMsgBuf != NULL) { - lmsg = malloc(sizeof(msg_list_t)); - lmsg->code = error; - lmsg->msg = lpMsgBuf; - ISC_LIST_APPEND(errormsg_list, lmsg, link); + if (lpMsgBuf) { + lmsg = malloc(sizeof(*lmsg)); + if (lmsg) { + lmsg->code = error; + lmsg->msg = lpMsgBuf; + ISC_LIST_APPEND(errormsg_list, lmsg, link); + } } UNLOCK(&ErrorMsgLock); return (lpMsgBuf); } /* - * This routine checks the error value and calls the WSA Windows Sockets - * Error message function GetWSAErrorMessage below if it's within that range - * since those messages are not available in the system error messages. + * Free the memory allocated for error messages during shutdown to + * increase the utility of _DEBUG MS CRT's malloc leak checks. + * + * ntservice_exit() calls this routine during shutdown. (ntservice.c) + */ +#ifdef _DEBUG +void +FormatErrorFreeMem(void) { + msg_list_t *entry; + /* + * Delete the lock used to protect the error message list as + * a cheap way of catching FormatError calls after this + * routine has torn things down for termination. + */ + DeleteCriticalSection(&ErrorMsgLock); + + entry = ISC_LIST_HEAD(errormsg_list); + while (entry) { + ISC_LIST_DEQUEUE(errormsg_list, entry, link); + + /* Memory from FormatMessage goes back via LocalFree */ + LocalFree(entry->msg); + free(entry); + + entry = ISC_LIST_HEAD(errormsg_list); + } +} +#endif /* _DEBUG */ + +/* + * This routine checks the error value and calls the WSA Windows + * Sockets Error message function GetWSAErrorMessage below if it's + * within that range since those messages are not available in the + * system error messages in Windows NT 4 and earlier Windows. With + * Windows 2000 and later, FormatMessage will translate socket errors, + * so GetWSAErrorMessage can be removed once Windows NT is no longer + * supported at runtime. */ static char * isc__NTstrerror(int err) { char *retmsg = NULL; - /* Copy the error value first in case of other errors */ - DWORD errval = err; - /* Get the Winsock2 error messages */ - if (errval >= WSABASEERR && errval <= (WSABASEERR + 1015)) { - retmsg = GetWSAErrorMessage(errval); - if (retmsg != NULL) - return (retmsg); + if (err >= WSABASEERR && err <= (WSABASEERR + 1015)) { + retmsg = GetWSAErrorMessage(err); } /* - * If it's not one of the standard Unix error codes, - * try a system error message + * C4996 here is strerror may be unsafe, consider + * strerror_s instead. + * strerror_s copies the message into a caller-supplied buffer, + * which is not an option when providing a strerror() wrapper. */ - if (errval > (DWORD) _sys_nerr) { - return (FormatError(errval)); - } else { - return (_sys_errlist[errval]); + #if defined(_MSC_VER) + #pragma warning(push) + #pragma warning(disable: 4996) + #endif /* _MSC_VER */ + + if (!retmsg) { + retmsg = strerror(err); + } + + #if defined(_MSC_VER) + #pragma warning(pop) + #endif /* _MSC_VER */ + + if (!retmsg) { + /* + * Handle a few errno values MS strerror() doesn't, + * since they only generate a subset of errno values + * in the C runtime. These are used by RFC 2783 + * PPSAPI timepps.h functions in ntpd on Windows. + */ + + #ifndef ENXIO + #define ENXIO 6 + #endif + + #ifndef EOPNOTSUPP + #define EOPNOTSUPP 45 + #endif + + switch (err) { + case ENXIO: + retmsg = "Device not configured"; + break; + + case EOPNOTSUPP: + retmsg = "Operation not supported"; + break; + } } + /* + * Handle Windows GetLastError error codes. Note that some + * low-numbered Windows error codes conflict with the errno + * space and this routine will return the errno-related text + * in preference to the GetLastError/FormatMessage text for + * those conflicting values. + */ + if (!retmsg) { + retmsg = FormatError(err); + } + + return (retmsg); } /* @@ -191,8 +294,10 @@ NTperror(char *errmsg) { initialize(); msg = isc__NTstrerror(errval); + if (!msg) { + msg = "unrecognized errno"; + } fprintf(stderr, "%s: %s\n", errmsg, msg); - } /* @@ -409,6 +514,7 @@ GetWSAErrorMessage(int errval) { return (msg); } +#ifdef NTP_DEAD_CODE_GetCryptErrorMessage /* * These error messages are more informative about CryptAPI Errors than the * standard error messages @@ -497,4 +603,4 @@ GetCryptErrorMessage(int errval) { } return msg; } - +#endif diff --git a/ports/winnt/libntp/randfile.c b/ports/winnt/libntp/randfile.c index 75a184b052..5e196a825d 100644 --- a/ports/winnt/libntp/randfile.c +++ b/ports/winnt/libntp/randfile.c @@ -3,12 +3,7 @@ * so that OpenSSL can work properly and securely. */ -/* Skip asynch rpc inclusion */ -#ifndef __RPCASYNC_H__ -#define __RPCASYNC_H__ -#endif - -#include +#include #include #include diff --git a/ports/winnt/ntpd/ntservice.c b/ports/winnt/ntpd/ntservice.c index 4aaf8c09e8..2624711abc 100644 --- a/ports/winnt/ntpd/ntservice.c +++ b/ports/winnt/ntpd/ntservice.c @@ -27,9 +27,6 @@ #include "clockstuff.h" #include "ntp_iocompletionport.h" #include "isc/win32os.h" -#ifdef DEBUG -#include -#endif /* Handle to SCM for updating service status */ @@ -44,6 +41,12 @@ extern volatile int debug; void uninit_io_completion_port(); int ntpdmain(int argc, char *argv[]); +/* + * libisc\nt_strerror.c + */ +#ifdef DEBUG +void FormatErrorFreeMem(void); +#endif /* * Forward declarations */ @@ -145,6 +148,18 @@ ntservice_init() { SetConsoleTitle(ConsoleTitle); } + #ifdef _DEBUG + /* ask the runtime to dump memory leaks at exit */ + _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF); + #ifdef WANT_LEAK_CHECK_ON_STDERR_TOO + /* hart: I haven't seen this work, running ntpd.exe -n from a shell */ + /* to both a file and the debugger output window */ + _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG); + /* the file being stderr */ + _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR); + #endif + #endif /* _DEBUG */ + atexit( ntservice_exit ); } @@ -173,11 +188,11 @@ ntservice_exit( void ) reset_winnt_time(); - msyslog(LOG_INFO, "ntservice: The Network Time Protocol Service has stopped."); + msyslog(LOG_INFO, "ntservice: The Network Time Protocol Service is stopping."); -# ifdef DEBUG - _CrtDumpMemoryLeaks(); -# endif +#ifdef _DEBUG + FormatErrorFreeMem(); +#endif if (!foreground) { /* service mode, need to have the service_main routine