autofs-5.0.4-code-analysis-corrections.patch

   1 autofs-5.0.4 - code analysis corrections
   2
   3 From: Ian Kent <raven@themaw.net>
   4
   5 Several mistakes have been reported by Paul Wankadia <junyer@google.com>:
   6 - a malloc(3) allocation return was not being checked in make_fullpath().
   7 - a double free and a use after free was identified in lookup_prune_cache().
   8 - off-by-one buffer overflow in lib/macros.c:macro_parse_globalvar().
   9 - several potential buffer overflows in modules/parse_hesiod.c.
  10 - double free in daemon/indirect.c:do_mount_autofs_indirect().
  11 - bogus struct name used for sizeof in lib/cache.c:cache_init() and
  12   lib/cache.c:cache_init_null_cache().
  13 - in daemon/direct.c:handle_packet_expire_direct master_unlock_mutex() not
  14   needed and mutexes not unlocked for file descriptor fail case.
  15 - in modules/lookup_multi.c:lookup_init() struct module_info array not
  16   checked before free for allocation failure case.
  17 - in modules/lookup_program.c:lookup_mount() mapent not freed on cache update failure.
  18 - in modules/mount_nfs.c allocation of mount location not checked.
  19 - in modules/parse_sun.c:parse_mapent() mount location not freed on syntax error.
  20 - in modules/parse_sun.c:parse_mount() mount location not freed on syntax error.
  21 - in modules/parse_sun.c:parse_init() a malloc is not checked and the
  22   handling of the fail case is poor.
  23 - in lib/mounts.c:tree_make_mnt_tree() variable ent is not freed on ent->path
  24   alloc fail.
  25 - in modules/replicated.c:add_host() NULL pointer dereference.
  26 - add missing pthread_attr_destroy() in lib/alarm.c:alarm_start_handler().
  27 - add missing pthread_attr_destroy() in daemon/state.c:st_start_handler().
  28 - add missing fclose() in lib/defaults.c:*defaults_get_searchdns().
  29 - add missing close()es in modules/mount_changer.c:swapCD().
  30 ---
  31
  32  daemon/direct.c          |    6 ++-
  33  daemon/indirect.c        |    3 +-
  34  daemon/lookup.c          |   20 +++++-------
  35  daemon/state.c           |    6 ++-
  36  lib/alarm.c              |    6 ++-
  37  lib/cache.c              |    4 +-
  38  lib/defaults.c           |    1 +
  39  lib/macros.c             |    2 +
  40  lib/mounts.c             |    5 ++-
  41  modules/lookup_multi.c   |   15 +++++----
  42  modules/lookup_program.c |    4 ++
  43  modules/mount_changer.c  |    2 +
  44  modules/mount_nfs.c      |    5 +++
  45  modules/parse_hesiod.c   |   79 ++++++++++++++++++++++++++++++++++++++++------
  46  modules/parse_sun.c      |   18 ++++++----
  47  modules/replicated.c     |    2 +
  48  16 files changed, 123 insertions(+), 55 deletions(-)
  49
  50
  51 diff --git a/daemon/direct.c b/daemon/direct.c
  52 index 2d979f1..fc3c969 100644
  53 --- a/daemon/direct.c
  54 +++ b/daemon/direct.c
  55 @@ -1088,7 +1088,6 @@ int handle_packet_expire_direct(struct autofs_point *ap, autofs_packet_expire_di
  56                 crit(ap->logopt, "can't find map entry for (%lu,%lu)",
  57                     (unsigned long) pkt->dev, (unsigned long) pkt->ino);
  58                 master_source_unlock(ap->entry);
  59 -               master_mutex_unlock();
  60                 pthread_setcancelstate(state, NULL);
  61                 return 1;
  62         }
  63 @@ -1098,8 +1097,9 @@ int handle_packet_expire_direct(struct autofs_point *ap, autofs_packet_expire_di
  64                 int ioctlfd;
  65                 ops->open(ap->logopt, &ioctlfd, me->dev, me->key);
  66                 if (ioctlfd == -1) {
  67 -                       crit(ap->logopt, "can't open ioctlfd for %s",
  68 -                            me->key);
  69 +                       crit(ap->logopt, "can't open ioctlfd for %s", me->key);
  70 +                       cache_unlock(mc);
  71 +                       master_source_unlock(ap->entry);
  72                         pthread_setcancelstate(state, NULL);
  73                         return 1;
  74                 }
  75 diff --git a/daemon/indirect.c b/daemon/indirect.c
  76 index 2ccbc53..f40c393 100644
  77 --- a/daemon/indirect.c
  78 +++ b/daemon/indirect.c
  79 @@ -159,6 +159,7 @@ static int do_mount_autofs_indirect(struct autofs_point *ap, const char *root)
  80         }
  81
  82         free(options);
  83 +       options = NULL;
  84
  85         ret = stat(root, &st);
  86         if (ret == -1) {
  87 @@ -167,8 +168,6 @@ static int do_mount_autofs_indirect(struct autofs_point *ap, const char *root)
  88                 goto out_umount;
  89         }
  90
  91 -       options = NULL;
  92 -
  93         if (ops->open(ap->logopt, &ap->ioctlfd, st.st_dev, root)) {
  94                 crit(ap->logopt,
  95                      "failed to create ioctl fd for autofs path %s", ap->path);
  96 diff --git a/daemon/lookup.c b/daemon/lookup.c
  97 index e034348..fd2ce55 100644
  98 --- a/daemon/lookup.c
  99 +++ b/daemon/lookup.c
 100 @@ -1001,12 +1001,16 @@ static char *make_fullpath(const char *root, const char *key)
 101                 if (l > KEY_MAX_LEN)
 102                         return NULL;
 103                 path = malloc(l);
 104 +               if (!path)
 105 +                       return NULL;
 106                 strcpy(path, key);
 107         } else {
 108                 l = strlen(key) + 1 + strlen(root) + 1;
 109                 if (l > KEY_MAX_LEN)
 110                         return NULL;
 111                 path = malloc(l);
 112 +               if (!path)
 113 +                       return NULL;
 114                 sprintf(path, "%s/%s", root, key);
 115         }
 116         return path;
 117 @@ -1076,10 +1080,6 @@ int lookup_prune_cache(struct autofs_point *ap, time_t age)
 118                         this = cache_lookup_distinct(mc, key);
 119                         if (!this) {
 120                                 cache_unlock(mc);
 121 -                               free(key);
 122 -                               if (next_key)
 123 -                                       free(next_key);
 124 -                               free(path);
 125                                 goto next;
 126                         }
 127
 128 @@ -1097,18 +1097,14 @@ int lookup_prune_cache(struct autofs_point *ap, time_t age)
 129                         }
 130                         cache_unlock(mc);
 131
 132 -                       if (!next_key) {
 133 -                               free(key);
 134 -                               free(path);
 135 -                               cache_readlock(mc);
 136 -                               continue;
 137 -                       }
 138  next:
 139                         cache_readlock(mc);
 140 -                       me = cache_lookup_distinct(mc, next_key);
 141 +                       if (next_key) {
 142 +                               me = cache_lookup_distinct(mc, next_key);
 143 +                               free(next_key);
 144 +                       }
 145                         free(key);
 146                         free(path);
 147 -                       free(next_key);
 148                 }
 149                 pthread_cleanup_pop(1);
 150                 map->stale = 0;
 151 diff --git a/daemon/state.c b/daemon/state.c
 152 index cd63be1..606743b 100644
 153 --- a/daemon/state.c
 154 +++ b/daemon/state.c
 155 @@ -1140,9 +1140,9 @@ int st_start_handler(void)
 156         }
 157
 158         status = pthread_create(&thid, pattrs, st_queue_handler, NULL);
 159 -       if (status)
 160 -               return 0;
 161
 162 -       return 1;
 163 +       pthread_attr_destroy(pattrs);
 164 +
 165 +       return !status;
 166  }
 167
 168 diff --git a/lib/alarm.c b/lib/alarm.c
 169 index 1e32291..46df38a 100755
 170 --- a/lib/alarm.c
 171 +++ b/lib/alarm.c
 172 @@ -238,9 +238,9 @@ int alarm_start_handler(void)
 173         }
 174
 175         status = pthread_create(&thid, pattrs, alarm_handler, NULL);
 176 -       if (status)
 177 -               return 0;
 178
 179 -       return 1;
 180 +       pthread_attr_destroy(pattrs);
 181 +
 182 +       return !status;
 183  }
 184
 185 diff --git a/lib/cache.c b/lib/cache.c
 186 index edb3192..4cb4582 100644
 187 --- a/lib/cache.c
 188 +++ b/lib/cache.c
 189 @@ -192,7 +192,7 @@ struct mapent_cache *cache_init(struct autofs_point *ap, struct map_source *map)
 190
 191         mc->size = defaults_get_map_hash_table_size();
 192
 193 -       mc->hash = malloc(mc->size * sizeof(struct entry *));
 194 +       mc->hash = malloc(mc->size * sizeof(struct mapent *));
 195         if (!mc->hash) {
 196                 free(mc);
 197                 return NULL;
 198 @@ -243,7 +243,7 @@ struct mapent_cache *cache_init_null_cache(struct master *master)
 199
 200         mc->size = NULL_MAP_HASHSIZE;
 201
 202 -       mc->hash = malloc(mc->size * sizeof(struct entry *));
 203 +       mc->hash = malloc(mc->size * sizeof(struct mapent *));
 204         if (!mc->hash) {
 205                 free(mc);
 206                 return NULL;
 207 diff --git a/lib/defaults.c b/lib/defaults.c
 208 index 0d39716..e507a59 100644
 209 --- a/lib/defaults.c
 210 +++ b/lib/defaults.c
 211 @@ -565,6 +565,7 @@ struct ldap_searchdn *defaults_get_searchdns(void)
 212
 213                         if (!new) {
 214                                 defaults_free_searchdns(sdn);
 215 +                               fclose(f);
 216                                 return NULL;
 217                         }
 218
 219 diff --git a/lib/macros.c b/lib/macros.c
 220 index 85f9cd3..32b70bf 100644
 221 --- a/lib/macros.c
 222 +++ b/lib/macros.c
 223 @@ -165,7 +165,7 @@ int macro_parse_globalvar(const char *define)
 224         char buf[MAX_MACRO_STRING];
 225         char *pbuf, *value;
 226
 227 -       if (strlen(define) > MAX_MACRO_STRING)
 228 +       if (strlen(define) >= MAX_MACRO_STRING)
 229                 return 0;
 230
 231         strcpy(buf, define);
 232 diff --git a/lib/mounts.c b/lib/mounts.c
 233 index b98e1a4..08ca4e3 100644
 234 --- a/lib/mounts.c
 235 +++ b/lib/mounts.c
 236 @@ -257,10 +257,10 @@ struct mnt_list *get_mnt_list(const char *table, const char *path, int include)
 237
 238                 if (mptr == list)
 239                         list = ent;
 240 +               else
 241 +                       last->next = ent;
 242
 243                 ent->next = mptr;
 244 -               if (last)
 245 -                       last->next = ent;
 246
 247                 ent->path = malloc(len + 1);
 248                 if (!ent->path) {
 249 @@ -705,6 +705,7 @@ struct mnt_list *tree_make_mnt_tree(const char *table, const char *path)
 250                 ent->path = malloc(len + 1);
 251                 if (!ent->path) {
 252                         endmntent(tab);
 253 +                       free(ent);
 254                         tree_free_mnt_tree(tree);
 255                         return NULL;
 256                 }
 257 diff --git a/modules/lookup_multi.c b/modules/lookup_multi.c
 258 index 1bf2e0a..6ec8434 100644
 259 --- a/modules/lookup_multi.c
 260 +++ b/modules/lookup_multi.c
 261 @@ -212,14 +212,15 @@ nomem:
 262         logerr(MODPREFIX "error: %s", estr);
 263  error_out:
 264         if (ctxt) {
 265 -               for (i = 0; i < ctxt->n; i++) {
 266 -                       if (ctxt->m[i].mod)
 267 -                               close_lookup(ctxt->m[i].mod);
 268 -                       if (ctxt->m[i].argv)
 269 -                               free_argv(ctxt->m[i].argc, ctxt->m[i].argv);
 270 -               }
 271 -               if (ctxt->m)
 272 +               if (ctxt->m) {
 273 +                       for (i = 0; i < ctxt->n; i++) {
 274 +                               if (ctxt->m[i].mod)
 275 +                                       close_lookup(ctxt->m[i].mod);
 276 +                               if (ctxt->m[i].argv)
 277 +                                       free_argv(ctxt->m[i].argc, ctxt->m[i].argv);
 278 +                       }
 279                         free(ctxt->m);
 280 +               }
 281                 if (ctxt->argl)
 282                         free(ctxt->argl);
 283                 free(ctxt);
 284 diff --git a/modules/lookup_program.c b/modules/lookup_program.c
 285 index 9878936..5b295a5 100644
 286 --- a/modules/lookup_program.c
 287 +++ b/modules/lookup_program.c
 288 @@ -396,8 +396,10 @@ next:
 289         cache_writelock(mc);
 290         ret = cache_update(mc, source, name, mapent, time(NULL));
 291         cache_unlock(mc);
 292 -       if (ret == CHE_FAIL)
 293 +       if (ret == CHE_FAIL) {
 294 +               free(mapent);
 295                 return NSS_STATUS_UNAVAIL;
 296 +       }
 297
 298         debug(ap->logopt, MODPREFIX "%s -> %s", name, mapent);
 299
 300 diff --git a/modules/mount_changer.c b/modules/mount_changer.c
 301 index 92bb72b..c30190d 100644
 302 --- a/modules/mount_changer.c
 303 +++ b/modules/mount_changer.c
 304 @@ -162,6 +162,7 @@ int swapCD(const char *device, const char *slotName)
 305                 logerr(MODPREFIX
 306                       "Device %s is not an ATAPI compliant CD changer.",
 307                       device);
 308 +               close(fd);
 309                 return 1;
 310         }
 311
 312 @@ -169,6 +170,7 @@ int swapCD(const char *device, const char *slotName)
 313         slot = ioctl(fd, CDROM_SELECT_DISC, slot);
 314         if (slot < 0) {
 315                 logerr(MODPREFIX "CDROM_SELECT_DISC failed");
 316 +               close(fd);
 317                 return 1;
 318         }
 319
 320 diff --git a/modules/mount_nfs.c b/modules/mount_nfs.c
 321 index 20732f8..6f54f47 100644
 322 --- a/modules/mount_nfs.c
 323 +++ b/modules/mount_nfs.c
 324 @@ -221,6 +221,11 @@ int mount_mount(struct autofs_point *ap, const char *root, const char *name, int
 325                 /* Not a local host - do an NFS mount */
 326
 327                 loc = malloc(strlen(this->name) + 1 + strlen(this->path) + 1);
 328 +               if (!loc) {
 329 +                       char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
 330 +                       error(ap->logopt, "malloc: %s", estr);
 331 +                       return 1;
 332 +               }
 333                 strcpy(loc, this->name);
 334                 strcat(loc, ":");
 335                 strcat(loc, this->path);
 336 diff --git a/modules/parse_hesiod.c b/modules/parse_hesiod.c
 337 index d5bb0f4..7a6a57d 100644
 338 --- a/modules/parse_hesiod.c
 339 +++ b/modules/parse_hesiod.c
 340 @@ -46,6 +46,12 @@ static int parse_afs(struct autofs_point *ap,
 341
 342         /* Isolate the source for this AFS fs. */
 343         for (i = 0; (!isspace(p[i]) && i < source_len); i++) {
 344 +               if (!p[i]) {
 345 +                       error(ap->logopt, MODPREFIX
 346 +                             "unexpeced end of input looking for AFS "
 347 +                             "source: %s", p);
 348 +                       return 1;
 349 +               }
 350                 source[i] = p[i];
 351         }
 352
 353 @@ -56,8 +62,14 @@ static int parse_afs(struct autofs_point *ap,
 354         while ((*p) && (isspace(*p)))
 355                 p++;
 356
 357 -       /* Isolate the source for this AFS fs. */
 358 +       /* Isolate the options for this AFS fs. */
 359         for (i = 0; (!isspace(p[i]) && i < options_len); i++) {
 360 +               if (!p[i]) {
 361 +                       error(ap->logopt, MODPREFIX
 362 +                             "unexpeced end of input looking for AFS "
 363 +                             "options: %s", p);
 364 +                       return 1;
 365 +               }
 366                 options[i] = p[i];
 367         }
 368         options[i] = 0;
 369 @@ -106,6 +118,12 @@ static int parse_nfs(struct autofs_point *ap,
 370
 371         /* Isolate the remote mountpoint for this NFS fs. */
 372         for (i = 0; (!isspace(p[i]) && i < (int) sizeof(mount)); i++) {
 373 +               if (!p[i]) {
 374 +                       error(ap->logopt, MODPREFIX
 375 +                             "unexpeced end of input looking for NFS "
 376 +                             "mountpoint: %s", p);
 377 +                       return 1;
 378 +               }
 379                 mount[i] = p[i];
 380         }
 381
 382 @@ -118,15 +136,26 @@ static int parse_nfs(struct autofs_point *ap,
 383
 384         /* Isolate the remote host. */
 385         for (i = 0; (!isspace(p[i]) && i < source_len); i++) {
 386 +               if (!p[i]) {
 387 +                       error(ap->logopt, MODPREFIX
 388 +                             "unexpeced end of input looking for NFS "
 389 +                             "host: %s", p);
 390 +                       return 1;
 391 +               }
 392                 source[i] = p[i];
 393         }
 394
 395         source[i] = 0;
 396         p += i;
 397
 398 +       if (strlen(source) + strlen(mount) + 2 > source_len) {
 399 +               error(ap->logopt, MODPREFIX "entry too log for mount source");
 400 +               return 1;
 401 +       }
 402 +
 403         /* Append ":mountpoint" to the source to get "host:mountpoint". */
 404 -       strncat(source, ":", source_len);
 405 -       strncat(source, mount, source_len);
 406 +       strcat(source, ":");
 407 +       strcat(source, mount);
 408
 409         /* Skip whitespace. */
 410         while ((*p) && (isspace(*p)))
 411 @@ -134,6 +163,12 @@ static int parse_nfs(struct autofs_point *ap,
 412
 413         /* Isolate the mount options. */
 414         for (i = 0; (!isspace(p[i]) && i < options_len); i++) {
 415 +               if (!p[i]) {
 416 +                       error(ap->logopt, MODPREFIX
 417 +                             "unexpeced end of input looking for NFS "
 418 +                             "mount options: %s", p);
 419 +                       return 1;
 420 +               }
 421                 options[i] = p[i];
 422         }
 423         options[i] = 0;
 424 @@ -178,6 +213,12 @@ static int parse_generic(struct autofs_point *ap,
 425
 426         /* Isolate the source for this fs. */
 427         for (i = 0; (!isspace(p[i]) && i < source_len); i++) {
 428 +               if (!p[i]) {
 429 +                       error(ap->logopt, MODPREFIX
 430 +                             "unexpeced end of input looking for generic "
 431 +                             "mount source: %s", p);
 432 +                       return 1;
 433 +               }
 434                 source[i] = p[i];
 435         }
 436
 437 @@ -190,6 +231,12 @@ static int parse_generic(struct autofs_point *ap,
 438
 439         /* Isolate the mount options. */
 440         for (i = 0; (!isspace(p[i]) && i < options_len); i++) {
 441 +               if (!p[i]) {
 442 +                       error(ap->logopt, MODPREFIX
 443 +                             "unexpeced end of input looking for generic "
 444 +                             "mount options: %s", p);
 445 +                       return 1;
 446 +               }
 447                 options[i] = p[i];
 448         }
 449         options[i] = 0;
 450 @@ -227,6 +274,7 @@ int parse_mount(struct autofs_point *ap, const char *name,
 451         char options[HESIOD_LEN + 1];
 452         char *q;
 453         const char *p;
 454 +       int ret;
 455
 456         ap->entry->current = NULL;
 457         master_source_current_signal(ap->entry);
 458 @@ -250,19 +298,28 @@ int parse_mount(struct autofs_point *ap, const char *name,
 459                 return 1;
 460         /* If it's an AFS fs... */
 461         } else if (!strcasecmp(fstype, "afs"))
 462 -               parse_afs(ap, mapent, name, name_len,
 463 -                         source, sizeof(source), options, sizeof(options));
 464 +               ret = parse_afs(ap, mapent, name, name_len,
 465 +                               source, sizeof(source), options,
 466 +                               sizeof(options));
 467         /* If it's NFS... */
 468         else if (!strcasecmp(fstype, "nfs"))
 469 -               parse_nfs(ap, mapent, name, name_len,
 470 -                         source, sizeof(source), options, sizeof(options));
 471 +               ret = parse_nfs(ap, mapent, name, name_len,
 472 +                               source, sizeof(source), options,
 473 +                               sizeof(options));
 474         /* Punt. */
 475         else
 476 -               parse_generic(ap, mapent, name, name_len, source, sizeof(source),
 477 -                             options, sizeof(options));
 478 +               ret = parse_generic(ap, mapent, name, name_len,
 479 +                                   source, sizeof(source), options,
 480 +                                   sizeof(options));
 481
 482 -       debug(ap->logopt,
 483 -             MODPREFIX "mount %s is type %s from %s", name, fstype, source);
 484 +       if (ret) {
 485 +               error(ap->logopt, MODPREFIX "failed to parse entry");
 486 +               return 1;
 487 +       } else {
 488 +               debug(ap->logopt,
 489 +                     MODPREFIX "mount %s is type %s from %s",
 490 +                     name, fstype, source);
 491 +       }
 492
 493         return do_mount(ap, ap->path, name, name_len, source, fstype, options);
 494  }
 495 diff --git a/modules/parse_sun.c b/modules/parse_sun.c
 496 index 72e51e2..ed73e46 100644
 497 --- a/modules/parse_sun.c
 498 +++ b/modules/parse_sun.c
 499 @@ -379,15 +379,17 @@ int parse_init(int argc, const char *const *argv, void **context)
 500                         if (ctxt->optstr) {
 501                                 noptstr =
 502                                     (char *) realloc(ctxt->optstr, optlen + len + 2);
 503 -                               if (!noptstr)
 504 -                                       break;
 505 -                               noptstr[optlen] = ',';
 506 -                               strcpy(noptstr + optlen + 1, argv[i] + offset);
 507 -                               optlen += len + 1;
 508 +                               if (noptstr) {
 509 +                                       noptstr[optlen] = ',';
 510 +                                       strcpy(noptstr + optlen + 1, argv[i] + offset);
 511 +                                       optlen += len + 1;
 512 +                               }
 513                         } else {
 514                                 noptstr = (char *) malloc(len + 1);
 515 -                               strcpy(noptstr, argv[i] + offset);
 516 -                               optlen = len;
 517 +                               if (noptstr) {
 518 +                                       strcpy(noptstr, argv[i] + offset);
 519 +                                       optlen = len;
 520 +                               }
 521                         }
 522                         if (!noptstr) {
 523                                 char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
 524 @@ -895,6 +897,7 @@ static int parse_mapent(const char *ent, char *g_options, char **options, char *
 525         if (*p == '/') {
 526                 warn(logopt, MODPREFIX "error location begins with \"/\"");
 527                 free(myoptions);
 528 +               free(loc);
 529                 return 0;
 530         }
 531
 532 @@ -1636,6 +1639,7 @@ int parse_mount(struct autofs_point *ap, const char *name,
 533                 /* Location can't begin with a '/' */
 534                 if (*p == '/') {
 535                         free(options);
 536 +                       free(loc);
 537                         warn(ap->logopt,
 538                               MODPREFIX "error location begins with \"/\"");
 539                         return 1;
 540 diff --git a/modules/replicated.c b/modules/replicated.c
 541 index 63829a2..835af97 100644
 542 --- a/modules/replicated.c
 543 +++ b/modules/replicated.c
 544 @@ -304,7 +304,7 @@ static int add_host(struct host **list, struct host *host)
 545  {
 546         struct host *this, *last;
 547
 548 -       if (!list) {
 549 +       if (!*list) {
 550                 *list = host;
 551                 return 1;
 552         }