On Thu, 2012-08-02 at 19:16 -0300, Leonardo Chiquitto wrote:
> Hello,
>
> I'm investigating a segmentation fault in the automounter, caused by invalid
> memory access in lookup_prune_one_cache(). I'd appreciate your opinion
> of the analysis below. The crash signature is:
>
> #0 lookup_prune_one_cache (ap=0x55555569d1b0, mc=0x55555569d360,
> age=1342705333) at lookup.c:1113
> 1113 rmdir_path(ap, path, this->dev);
>
> (gdb) print this
> $1 = (struct mapent *) 0x2aaaaccf49f0
> (gdb) print *this
> Cannot access memory at address 0x2aaaaccf49f0
>
> From lookup_prune_one_cache():
> [line numbers are from the version that crashed, not the latest git]
>
> 1095 cache_writelock(mc);
> 1096 this = cache_lookup_distinct(mc, key); // lookup key in the hash
> // table mc->hash
> 1097 if (!this) {
> 1098 cache_unlock(mc);
> 1099 goto next;
> 1100 }
> 1101
> 1102 if (valid) // here we know valid == 0
> 1103 cache_delete(mc, key);
> 1104 else if (!is_mounted(_PROC_MOUNTS, path, MNTS_AUTOFS)) {
> // here we know the path is
> // currently not mounted
> 1105 status = CHE_FAIL;
> 1106 if (this->ioctlfd == -1) // here "this" still points to
> // valid memory
> // if this->ioctl == -1, it deletes this key from
> // the hash table and free the memory:
> 1107 status = cache_delete(mc, key);
> 1108 if (status != CHE_FAIL) {
> 1109 if (ap->type == LKP_INDIRECT) {
> 1110 if (ap->flags & MOUNT_FLAG_GHOST)
> 1111 rmdir_path(ap, path, ap->dev);
> 1112 } else
> 1113 rmdir_path(ap, path, this->dev); // crashes here:
> // "this" points
> // to freed mem.
Yes, that's an obvious stupid mistake.
> 1114 }
> 1115 }
> 1116 cache_unlock(mc);
>
> I'd like to understand if using "this->dev" instead of "ap->dev" in the second
> rmdir_path() call is indeed correct or if it should be "ap->dev" too.
That's a good question and I've had cause to think about it recently.
But in this case it's an actual direct mount (ie. not offset mount in a
multi-mount) so ap->dev cannot hold the device id of the mount since
there may be many of them. So it must be stored in the cache entry.
This is probably what's needed.
autofs-5.0.7 - fix use devid after free
From: Ian Kent <ikent@xxxxxxxxxx>
Fix an obvious use after free mistake in lookup_prune_one_cache().
---
daemon/lookup.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/daemon/lookup.c b/daemon/lookup.c
index 7909536..e3d9536 100644
--- a/daemon/lookup.c
+++ b/daemon/lookup.c
@@ -1103,15 +1103,18 @@ void lookup_prune_one_cache(struct autofs_point *ap, struct mapent_cache *mc, ti
if (valid)
cache_delete(mc, key);
else if (!is_mounted(_PROC_MOUNTS, path, MNTS_AUTOFS)) {
+ dev_t devid = ap->dev;
status = CHE_FAIL;
+ if (ap->type == LKP_DIRECT)
+ devid = this->dev;
if (this->ioctlfd == -1)
status = cache_delete(mc, key);
if (status != CHE_FAIL) {
if (ap->type == LKP_INDIRECT) {
if (ap->flags & MOUNT_FLAG_GHOST)
- rmdir_path(ap, path, ap->dev);
+ rmdir_path(ap, path, devid);
} else
- rmdir_path(ap, path, this->dev);
+ rmdir_path(ap, path, devid);
}
}
cache_unlock(mc);
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
