autofs-5.0.4 - code analysis corrections From: Ian Kent Several mistakes have been reported by Paul Wankadia : - a malloc(3) allocation return was not being checked in make_fullpath(). - a double free and a use after free was identified in lookup_prune_cache(). - off-by-one buffer overflow in lib/macros.c:macro_parse_globalvar(). - several potential buffer overflows in modules/parse_hesiod.c. - double free in daemon/indirect.c:do_mount_autofs_indirect(). - bogus struct name used for sizeof in lib/cache.c:cache_init() and lib/cache.c:cache_init_null_cache(). - in daemon/direct.c:handle_packet_expire_direct master_unlock_mutex() not needed and mutexes not unlocked for file descriptor fail case. - in modules/lookup_multi.c:lookup_init() struct module_info array not checked before free for allocation failure case. - in modules/lookup_program.c:lookup_mount() mapent not freed on cache update failure. - in modules/mount_nfs.c allocation of mount location not checked. - in modules/parse_sun.c:parse_mapent() mount location not freed on syntax error. - in modules/parse_sun.c:parse_mount() mount location not freed on syntax error. - in modules/parse_sun.c:parse_init() a malloc is not checked and the handling of the fail case is poor. - in lib/mounts.c:tree_make_mnt_tree() variable ent is not freed on ent->path alloc fail. - in modules/replicated.c:add_host() NULL pointer dereference. - add missing pthread_attr_destroy() in lib/alarm.c:alarm_start_handler(). - add missing pthread_attr_destroy() in daemon/state.c:st_start_handler(). - add missing fclose() in lib/defaults.c:*defaults_get_searchdns(). - add missing close()es in modules/mount_changer.c:swapCD(). --- daemon/direct.c | 6 ++- daemon/indirect.c | 3 +- daemon/lookup.c | 20 +++++------- daemon/state.c | 6 ++- lib/alarm.c | 6 ++- lib/cache.c | 4 +- lib/defaults.c | 1 + lib/macros.c | 2 + lib/mounts.c | 5 ++- modules/lookup_multi.c | 15 +++++---- modules/lookup_program.c | 4 ++ modules/mount_changer.c | 2 + modules/mount_nfs.c | 5 +++ modules/parse_hesiod.c | 79 ++++++++++++++++++++++++++++++++++++++++------ modules/parse_sun.c | 18 ++++++---- modules/replicated.c | 2 + 16 files changed, 123 insertions(+), 55 deletions(-) diff --git a/daemon/direct.c b/daemon/direct.c index 2d979f1..fc3c969 100644 --- a/daemon/direct.c +++ b/daemon/direct.c @@ -1088,7 +1088,6 @@ int handle_packet_expire_direct(struct autofs_point *ap, autofs_packet_expire_di crit(ap->logopt, "can't find map entry for (%lu,%lu)", (unsigned long) pkt->dev, (unsigned long) pkt->ino); master_source_unlock(ap->entry); - master_mutex_unlock(); pthread_setcancelstate(state, NULL); return 1; } @@ -1098,8 +1097,9 @@ int handle_packet_expire_direct(struct autofs_point *ap, autofs_packet_expire_di int ioctlfd; ops->open(ap->logopt, &ioctlfd, me->dev, me->key); if (ioctlfd == -1) { - crit(ap->logopt, "can't open ioctlfd for %s", - me->key); + crit(ap->logopt, "can't open ioctlfd for %s", me->key); + cache_unlock(mc); + master_source_unlock(ap->entry); pthread_setcancelstate(state, NULL); return 1; } diff --git a/daemon/indirect.c b/daemon/indirect.c index 2ccbc53..f40c393 100644 --- a/daemon/indirect.c +++ b/daemon/indirect.c @@ -159,6 +159,7 @@ static int do_mount_autofs_indirect(struct autofs_point *ap, const char *root) } free(options); + options = NULL; ret = stat(root, &st); if (ret == -1) { @@ -167,8 +168,6 @@ static int do_mount_autofs_indirect(struct autofs_point *ap, const char *root) goto out_umount; } - options = NULL; - if (ops->open(ap->logopt, &ap->ioctlfd, st.st_dev, root)) { crit(ap->logopt, "failed to create ioctl fd for autofs path %s", ap->path); diff --git a/daemon/lookup.c b/daemon/lookup.c index e034348..fd2ce55 100644 --- a/daemon/lookup.c +++ b/daemon/lookup.c @@ -1001,12 +1001,16 @@ static char *make_fullpath(const char *root, const char *key) if (l > KEY_MAX_LEN) return NULL; path = malloc(l); + if (!path) + return NULL; strcpy(path, key); } else { l = strlen(key) + 1 + strlen(root) + 1; if (l > KEY_MAX_LEN) return NULL; path = malloc(l); + if (!path) + return NULL; sprintf(path, "%s/%s", root, key); } return path; @@ -1076,10 +1080,6 @@ int lookup_prune_cache(struct autofs_point *ap, time_t age) this = cache_lookup_distinct(mc, key); if (!this) { cache_unlock(mc); - free(key); - if (next_key) - free(next_key); - free(path); goto next; } @@ -1097,18 +1097,14 @@ int lookup_prune_cache(struct autofs_point *ap, time_t age) } cache_unlock(mc); - if (!next_key) { - free(key); - free(path); - cache_readlock(mc); - continue; - } next: cache_readlock(mc); - me = cache_lookup_distinct(mc, next_key); + if (next_key) { + me = cache_lookup_distinct(mc, next_key); + free(next_key); + } free(key); free(path); - free(next_key); } pthread_cleanup_pop(1); map->stale = 0; diff --git a/daemon/state.c b/daemon/state.c index cd63be1..606743b 100644 --- a/daemon/state.c +++ b/daemon/state.c @@ -1140,9 +1140,9 @@ int st_start_handler(void) } status = pthread_create(&thid, pattrs, st_queue_handler, NULL); - if (status) - return 0; - return 1; + pthread_attr_destroy(pattrs); + + return !status; } diff --git a/lib/alarm.c b/lib/alarm.c index 1e32291..46df38a 100755 --- a/lib/alarm.c +++ b/lib/alarm.c @@ -238,9 +238,9 @@ int alarm_start_handler(void) } status = pthread_create(&thid, pattrs, alarm_handler, NULL); - if (status) - return 0; - return 1; + pthread_attr_destroy(pattrs); + + return !status; } diff --git a/lib/cache.c b/lib/cache.c index edb3192..4cb4582 100644 --- a/lib/cache.c +++ b/lib/cache.c @@ -192,7 +192,7 @@ struct mapent_cache *cache_init(struct autofs_point *ap, struct map_source *map) mc->size = defaults_get_map_hash_table_size(); - mc->hash = malloc(mc->size * sizeof(struct entry *)); + mc->hash = malloc(mc->size * sizeof(struct mapent *)); if (!mc->hash) { free(mc); return NULL; @@ -243,7 +243,7 @@ struct mapent_cache *cache_init_null_cache(struct master *master) mc->size = NULL_MAP_HASHSIZE; - mc->hash = malloc(mc->size * sizeof(struct entry *)); + mc->hash = malloc(mc->size * sizeof(struct mapent *)); if (!mc->hash) { free(mc); return NULL; diff --git a/lib/defaults.c b/lib/defaults.c index 0d39716..e507a59 100644 --- a/lib/defaults.c +++ b/lib/defaults.c @@ -565,6 +565,7 @@ struct ldap_searchdn *defaults_get_searchdns(void) if (!new) { defaults_free_searchdns(sdn); + fclose(f); return NULL; } diff --git a/lib/macros.c b/lib/macros.c index 85f9cd3..32b70bf 100644 --- a/lib/macros.c +++ b/lib/macros.c @@ -165,7 +165,7 @@ int macro_parse_globalvar(const char *define) char buf[MAX_MACRO_STRING]; char *pbuf, *value; - if (strlen(define) > MAX_MACRO_STRING) + if (strlen(define) >= MAX_MACRO_STRING) return 0; strcpy(buf, define); diff --git a/lib/mounts.c b/lib/mounts.c index b98e1a4..08ca4e3 100644 --- a/lib/mounts.c +++ b/lib/mounts.c @@ -257,10 +257,10 @@ struct mnt_list *get_mnt_list(const char *table, const char *path, int include) if (mptr == list) list = ent; + else + last->next = ent; ent->next = mptr; - if (last) - last->next = ent; ent->path = malloc(len + 1); if (!ent->path) { @@ -705,6 +705,7 @@ struct mnt_list *tree_make_mnt_tree(const char *table, const char *path) ent->path = malloc(len + 1); if (!ent->path) { endmntent(tab); + free(ent); tree_free_mnt_tree(tree); return NULL; } diff --git a/modules/lookup_multi.c b/modules/lookup_multi.c index 1bf2e0a..6ec8434 100644 --- a/modules/lookup_multi.c +++ b/modules/lookup_multi.c @@ -212,14 +212,15 @@ nomem: logerr(MODPREFIX "error: %s", estr); error_out: if (ctxt) { - for (i = 0; i < ctxt->n; i++) { - if (ctxt->m[i].mod) - close_lookup(ctxt->m[i].mod); - if (ctxt->m[i].argv) - free_argv(ctxt->m[i].argc, ctxt->m[i].argv); - } - if (ctxt->m) + if (ctxt->m) { + for (i = 0; i < ctxt->n; i++) { + if (ctxt->m[i].mod) + close_lookup(ctxt->m[i].mod); + if (ctxt->m[i].argv) + free_argv(ctxt->m[i].argc, ctxt->m[i].argv); + } free(ctxt->m); + } if (ctxt->argl) free(ctxt->argl); free(ctxt); diff --git a/modules/lookup_program.c b/modules/lookup_program.c index 9878936..5b295a5 100644 --- a/modules/lookup_program.c +++ b/modules/lookup_program.c @@ -396,8 +396,10 @@ next: cache_writelock(mc); ret = cache_update(mc, source, name, mapent, time(NULL)); cache_unlock(mc); - if (ret == CHE_FAIL) + if (ret == CHE_FAIL) { + free(mapent); return NSS_STATUS_UNAVAIL; + } debug(ap->logopt, MODPREFIX "%s -> %s", name, mapent); diff --git a/modules/mount_changer.c b/modules/mount_changer.c index 92bb72b..c30190d 100644 --- a/modules/mount_changer.c +++ b/modules/mount_changer.c @@ -162,6 +162,7 @@ int swapCD(const char *device, const char *slotName) logerr(MODPREFIX "Device %s is not an ATAPI compliant CD changer.", device); + close(fd); return 1; } @@ -169,6 +170,7 @@ int swapCD(const char *device, const char *slotName) slot = ioctl(fd, CDROM_SELECT_DISC, slot); if (slot < 0) { logerr(MODPREFIX "CDROM_SELECT_DISC failed"); + close(fd); return 1; } diff --git a/modules/mount_nfs.c b/modules/mount_nfs.c index 20732f8..6f54f47 100644 --- a/modules/mount_nfs.c +++ b/modules/mount_nfs.c @@ -221,6 +221,11 @@ int mount_mount(struct autofs_point *ap, const char *root, const char *name, int /* Not a local host - do an NFS mount */ loc = malloc(strlen(this->name) + 1 + strlen(this->path) + 1); + if (!loc) { + char *estr = strerror_r(errno, buf, MAX_ERR_BUF); + error(ap->logopt, "malloc: %s", estr); + return 1; + } strcpy(loc, this->name); strcat(loc, ":"); strcat(loc, this->path); diff --git a/modules/parse_hesiod.c b/modules/parse_hesiod.c index d5bb0f4..7a6a57d 100644 --- a/modules/parse_hesiod.c +++ b/modules/parse_hesiod.c @@ -46,6 +46,12 @@ static int parse_afs(struct autofs_point *ap, /* Isolate the source for this AFS fs. */ for (i = 0; (!isspace(p[i]) && i < source_len); i++) { + if (!p[i]) { + error(ap->logopt, MODPREFIX + "unexpeced end of input looking for AFS " + "source: %s", p); + return 1; + } source[i] = p[i]; } @@ -56,8 +62,14 @@ static int parse_afs(struct autofs_point *ap, while ((*p) && (isspace(*p))) p++; - /* Isolate the source for this AFS fs. */ + /* Isolate the options for this AFS fs. */ for (i = 0; (!isspace(p[i]) && i < options_len); i++) { + if (!p[i]) { + error(ap->logopt, MODPREFIX + "unexpeced end of input looking for AFS " + "options: %s", p); + return 1; + } options[i] = p[i]; } options[i] = 0; @@ -106,6 +118,12 @@ static int parse_nfs(struct autofs_point *ap, /* Isolate the remote mountpoint for this NFS fs. */ for (i = 0; (!isspace(p[i]) && i < (int) sizeof(mount)); i++) { + if (!p[i]) { + error(ap->logopt, MODPREFIX + "unexpeced end of input looking for NFS " + "mountpoint: %s", p); + return 1; + } mount[i] = p[i]; } @@ -118,15 +136,26 @@ static int parse_nfs(struct autofs_point *ap, /* Isolate the remote host. */ for (i = 0; (!isspace(p[i]) && i < source_len); i++) { + if (!p[i]) { + error(ap->logopt, MODPREFIX + "unexpeced end of input looking for NFS " + "host: %s", p); + return 1; + } source[i] = p[i]; } source[i] = 0; p += i; + if (strlen(source) + strlen(mount) + 2 > source_len) { + error(ap->logopt, MODPREFIX "entry too log for mount source"); + return 1; + } + /* Append ":mountpoint" to the source to get "host:mountpoint". */ - strncat(source, ":", source_len); - strncat(source, mount, source_len); + strcat(source, ":"); + strcat(source, mount); /* Skip whitespace. */ while ((*p) && (isspace(*p))) @@ -134,6 +163,12 @@ static int parse_nfs(struct autofs_point *ap, /* Isolate the mount options. */ for (i = 0; (!isspace(p[i]) && i < options_len); i++) { + if (!p[i]) { + error(ap->logopt, MODPREFIX + "unexpeced end of input looking for NFS " + "mount options: %s", p); + return 1; + } options[i] = p[i]; } options[i] = 0; @@ -178,6 +213,12 @@ static int parse_generic(struct autofs_point *ap, /* Isolate the source for this fs. */ for (i = 0; (!isspace(p[i]) && i < source_len); i++) { + if (!p[i]) { + error(ap->logopt, MODPREFIX + "unexpeced end of input looking for generic " + "mount source: %s", p); + return 1; + } source[i] = p[i]; } @@ -190,6 +231,12 @@ static int parse_generic(struct autofs_point *ap, /* Isolate the mount options. */ for (i = 0; (!isspace(p[i]) && i < options_len); i++) { + if (!p[i]) { + error(ap->logopt, MODPREFIX + "unexpeced end of input looking for generic " + "mount options: %s", p); + return 1; + } options[i] = p[i]; } options[i] = 0; @@ -227,6 +274,7 @@ int parse_mount(struct autofs_point *ap, const char *name, char options[HESIOD_LEN + 1]; char *q; const char *p; + int ret; ap->entry->current = NULL; master_source_current_signal(ap->entry); @@ -250,19 +298,28 @@ int parse_mount(struct autofs_point *ap, const char *name, return 1; /* If it's an AFS fs... */ } else if (!strcasecmp(fstype, "afs")) - parse_afs(ap, mapent, name, name_len, - source, sizeof(source), options, sizeof(options)); + ret = parse_afs(ap, mapent, name, name_len, + source, sizeof(source), options, + sizeof(options)); /* If it's NFS... */ else if (!strcasecmp(fstype, "nfs")) - parse_nfs(ap, mapent, name, name_len, - source, sizeof(source), options, sizeof(options)); + ret = parse_nfs(ap, mapent, name, name_len, + source, sizeof(source), options, + sizeof(options)); /* Punt. */ else - parse_generic(ap, mapent, name, name_len, source, sizeof(source), - options, sizeof(options)); + ret = parse_generic(ap, mapent, name, name_len, + source, sizeof(source), options, + sizeof(options)); - debug(ap->logopt, - MODPREFIX "mount %s is type %s from %s", name, fstype, source); + if (ret) { + error(ap->logopt, MODPREFIX "failed to parse entry"); + return 1; + } else { + debug(ap->logopt, + MODPREFIX "mount %s is type %s from %s", + name, fstype, source); + } return do_mount(ap, ap->path, name, name_len, source, fstype, options); } diff --git a/modules/parse_sun.c b/modules/parse_sun.c index 72e51e2..ed73e46 100644 --- a/modules/parse_sun.c +++ b/modules/parse_sun.c @@ -379,15 +379,17 @@ int parse_init(int argc, const char *const *argv, void **context) if (ctxt->optstr) { noptstr = (char *) realloc(ctxt->optstr, optlen + len + 2); - if (!noptstr) - break; - noptstr[optlen] = ','; - strcpy(noptstr + optlen + 1, argv[i] + offset); - optlen += len + 1; + if (noptstr) { + noptstr[optlen] = ','; + strcpy(noptstr + optlen + 1, argv[i] + offset); + optlen += len + 1; + } } else { noptstr = (char *) malloc(len + 1); - strcpy(noptstr, argv[i] + offset); - optlen = len; + if (noptstr) { + strcpy(noptstr, argv[i] + offset); + optlen = len; + } } if (!noptstr) { char *estr = strerror_r(errno, buf, MAX_ERR_BUF); @@ -895,6 +897,7 @@ static int parse_mapent(const char *ent, char *g_options, char **options, char * if (*p == '/') { warn(logopt, MODPREFIX "error location begins with \"/\""); free(myoptions); + free(loc); return 0; } @@ -1636,6 +1639,7 @@ int parse_mount(struct autofs_point *ap, const char *name, /* Location can't begin with a '/' */ if (*p == '/') { free(options); + free(loc); warn(ap->logopt, MODPREFIX "error location begins with \"/\""); return 1; diff --git a/modules/replicated.c b/modules/replicated.c index 63829a2..835af97 100644 --- a/modules/replicated.c +++ b/modules/replicated.c @@ -304,7 +304,7 @@ static int add_host(struct host **list, struct host *host) { struct host *this, *last; - if (!list) { + if (!*list) { *list = host; return 1; }