From efbf39642633b46e0c4c7bb9493b4887867a08db Mon Sep 17 00:00:00 2001 From: Kirk McKusick Date: Fri, 2 Mar 2018 04:34:53 +0000 Subject: [PATCH] This change is some refactoring of Mark Johnston's changes in r329375 to fix the memory leak that I introduced in r328426. Instead of trying to clear up the possible memory leak in all the clients, I ensure that it gets cleaned up in the source (e.g., ffs_sbget ensures that memory is always freed if it returns an error). The original change in r328426 was a bit sparse in its description. So I am expanding on its description here (thanks cem@ and rgrimes@ for your encouragement for my longer commit messages). In preparation for adding check hashing to superblocks, r328426 is a refactoring of the code to get the reading/writing of the superblock into one place. Unlike the cylinder group reading/writing which ends up in two places (ffs_getcg/ffs_geom_strategy in the kernel and cgget/cgput in libufs), I have the core superblock functions just in the kernel (ffs_sbfetch/ffs_sbput in ffs_subr.c which is already imported into utilities like fsck_ffs as well as libufs to implement sbget/sbput). The ffs_sbfetch and ffs_sbput functions take a function pointer to do the actual I/O for which there are four variants: ffs_use_bread / ffs_use_bwrite for the in-kernel filesystem g_use_g_read_data / g_use_g_write_data for kernel geom clients ufs_use_sa_read for the standalone code (stand/libsa/ufs.c but not stand/libsa/ufsread.c which is size constrained) use_pread / use_pwrite for libufs Uses of these interfaces are in the UFS filesystem, geoms journal & label, libsa changes, and libufs. They also permeate out into the filesystem utilities fsck_ffs, newfs, growfs, clri, dump, quotacheck, fsirand, fstyp, and quot. Some of these utilities should probably be converted to directly use libufs (like dumpfs was for example), but there does not seem to be much win in doing so. Tested by: Peter Holm (pho@) --- lib/libufs/sblock.c | 1 - stand/libsa/ufs.c | 3 +-- sys/geom/geom_io.c | 5 +++-- sys/geom/journal/g_journal_ufs.c | 6 +++--- sys/geom/label/g_label_ufs.c | 6 +++--- sys/ufs/ffs/ffs_subr.c | 37 ++++++++++++++++++++------------ sys/ufs/ffs/ffs_vfsops.c | 7 ++---- 7 files changed, 35 insertions(+), 30 deletions(-) diff --git a/lib/libufs/sblock.c b/lib/libufs/sblock.c index cea57acb9a6b..29ff258b6ebf 100644 --- a/lib/libufs/sblock.c +++ b/lib/libufs/sblock.c @@ -150,7 +150,6 @@ use_pread(void *devfd, off_t loc, void **bufp, int size) int fd; fd = *(int *)devfd; - free(*bufp); if ((*bufp = malloc(size)) == NULL) return (ENOSPC); if (pread(fd, *bufp, size, loc) != size) diff --git a/stand/libsa/ufs.c b/stand/libsa/ufs.c index 3f0d85be13c5..2cad803e3c73 100644 --- a/stand/libsa/ufs.c +++ b/stand/libsa/ufs.c @@ -518,7 +518,7 @@ ufs_open(upath, f) /* read super block */ twiddle(1); - if ((rc = ffs_sbget(f, &fs, -1, 0, ufs_use_sa_read)) != 0) + if ((rc = ffs_sbget(f, &fs, -1, "stand", ufs_use_sa_read)) != 0) goto out; fp->f_fs = fs; /* @@ -688,7 +688,6 @@ ufs_use_sa_read(void *devfd, off_t loc, void **bufp, int size) int error; f = (struct open_file *)devfd; - free(*bufp); if ((*bufp = malloc(size)) == NULL) return (ENOSPC); error = (f->f_dev->dv_strategy)(f->f_devdata, F_READ, loc / DEV_BSIZE, diff --git a/sys/geom/geom_io.c b/sys/geom/geom_io.c index 0f63b1ff31ec..9797884252ef 100644 --- a/sys/geom/geom_io.c +++ b/sys/geom/geom_io.c @@ -957,6 +957,9 @@ g_use_g_read_data(void *devfd, off_t loc, void **bufp, int size) { struct g_consumer *cp; + KASSERT(*bufp == NULL, + ("g_use_g_read_data: non-NULL *bufp %p\n", *bufp)); + cp = (struct g_consumer *)devfd; /* * Take care not to issue an invalid I/O request. The offset of @@ -966,8 +969,6 @@ g_use_g_read_data(void *devfd, off_t loc, void **bufp, int size) */ if (loc % cp->provider->sectorsize != 0) return (ENOENT); - if (*bufp != NULL) - g_free(*bufp); *bufp = g_read_data(cp, loc, size, NULL); if (*bufp == NULL) return (ENOENT); diff --git a/sys/geom/journal/g_journal_ufs.c b/sys/geom/journal/g_journal_ufs.c index 6672cceb8cb4..4488f0d45ea2 100644 --- a/sys/geom/journal/g_journal_ufs.c +++ b/sys/geom/journal/g_journal_ufs.c @@ -72,11 +72,11 @@ g_journal_ufs_dirty(struct g_consumer *cp) fs = NULL; if (SBLOCKSIZE % cp->provider->sectorsize != 0 || - ffs_sbget(cp, &fs, -1, NULL, g_use_g_read_data) != 0) { + ffs_sbget(cp, &fs, -1, M_GEOM, g_use_g_read_data) != 0) { GJ_DEBUG(0, "Cannot find superblock to mark file system %s " "as dirty.", cp->provider->name); - if (fs != NULL) - g_free(fs); + KASSERT(fs == NULL, + ("g_journal_ufs_dirty: non-NULL fs %p\n", fs)); return; } GJ_DEBUG(0, "clean=%d flags=0x%x", fs->fs_clean, fs->fs_flags); diff --git a/sys/geom/label/g_label_ufs.c b/sys/geom/label/g_label_ufs.c index f32ec4d55c19..feccc28b1f41 100644 --- a/sys/geom/label/g_label_ufs.c +++ b/sys/geom/label/g_label_ufs.c @@ -77,9 +77,9 @@ g_label_ufs_taste_common(struct g_consumer *cp, char *label, size_t size, int wh fs = NULL; if (SBLOCKSIZE % pp->sectorsize != 0 || - ffs_sbget(cp, &fs, -1, NULL, g_use_g_read_data) != 0) { - if (fs != NULL) - g_free(fs); + ffs_sbget(cp, &fs, -1, M_GEOM, g_use_g_read_data) != 0) { + KASSERT(fs == NULL, + ("g_label_ufs_taste_common: non-NULL fs %p\n",fs); return; } diff --git a/sys/ufs/ffs/ffs_subr.c b/sys/ufs/ffs/ffs_subr.c index 714a6ec009e9..34afc51a4726 100644 --- a/sys/ufs/ffs/ffs_subr.c +++ b/sys/ufs/ffs/ffs_subr.c @@ -151,6 +151,7 @@ static int readsuper(void *, struct fs **, off_t, int, * superblock. Memory is allocated for the superblock by the readfunc and * is returned. If filltype is non-NULL, additional memory is allocated * of type filltype and filled in with the superblock summary information. + * All memory is freed when any error is returned. * * If a superblock is found, zero is returned. Otherwise one of the * following error values is returned: @@ -172,16 +173,24 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsblock, int32_t *lp; char *buf; + fs = NULL; *fsp = NULL; if (altsblock != -1) { - if ((error = readsuper(devfd, fsp, altsblock, 1, - readfunc)) != 0) + if ((error = readsuper(devfd, &fs, altsblock, 1, + readfunc)) != 0) { + if (fs != NULL) + UFS_FREE(fs, filltype); return (error); + } } else { for (i = 0; sblock_try[i] != -1; i++) { - if ((error = readsuper(devfd, fsp, sblock_try[i], 0, + if ((error = readsuper(devfd, &fs, sblock_try[i], 0, readfunc)) == 0) break; + if (fs != NULL) { + UFS_FREE(fs, filltype); + fs = NULL; + } if (error == ENOENT) continue; return (error); @@ -189,21 +198,19 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsblock, if (sblock_try[i] == -1) return (ENOENT); } - /* - * If not filling in summary information, return. - */ - if (filltype == NULL) - return (0); /* * Read in the superblock summary information. */ - fs = *fsp; size = fs->fs_cssize; blks = howmany(size, fs->fs_fsize); if (fs->fs_contigsumsize > 0) size += fs->fs_ncg * sizeof(int32_t); size += fs->fs_ncg * sizeof(u_int8_t); - space = UFS_MALLOC(size, filltype, M_WAITOK); + /* When running in libufs or libsa, UFS_MALLOC may fail */ + if ((space = UFS_MALLOC(size, filltype, M_WAITOK)) == NULL) { + UFS_FREE(fs, filltype); + return (ENOSPC); + } fs->fs_csp = (struct csum *)space; for (i = 0; i < blks; i += fs->fs_frag) { size = fs->fs_bsize; @@ -213,9 +220,10 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsblock, error = (*readfunc)(devfd, dbtob(fsbtodb(fs, fs->fs_csaddr + i)), (void **)&buf, size); if (error) { - UFS_FREE(buf, filltype); + if (buf != NULL) + UFS_FREE(buf, filltype); UFS_FREE(fs->fs_csp, filltype); - fs->fs_csp = NULL; + UFS_FREE(fs, filltype); return (error); } memcpy(space, buf, size); @@ -231,6 +239,7 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsblock, size = fs->fs_ncg * sizeof(u_int8_t); fs->fs_contigdirs = (u_int8_t *)space; bzero(fs->fs_contigdirs, size); + *fsp = fs; return (0); } @@ -246,8 +255,6 @@ readsuper(void *devfd, struct fs **fsp, off_t sblockloc, int isaltsblk, int error; error = (*readfunc)(devfd, sblockloc, (void **)fsp, SBLOCKSIZE); - if (*fsp != NULL) - (*fsp)->fs_csp = NULL; /* Not yet any summary information */ if (error != 0) return (error); fs = *fsp; @@ -263,6 +270,8 @@ readsuper(void *devfd, struct fs **fsp, off_t sblockloc, int isaltsblk, fs->fs_bsize >= roundup(sizeof(struct fs), DEV_BSIZE)) { /* Have to set for old filesystems that predate this field */ fs->fs_sblockactualloc = sblockloc; + /* Not yet any summary information */ + fs->fs_csp = NULL; return (0); } return (ENOENT); diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c index 935661d10af6..79887041ba99 100644 --- a/sys/ufs/ffs/ffs_vfsops.c +++ b/sys/ufs/ffs/ffs_vfsops.c @@ -1075,14 +1075,11 @@ ffs_use_bread(void *devfd, off_t loc, void **bufp, int size) struct buf *bp; int error; - free(*bufp, M_UFSMNT); + KASSERT(*bufp == NULL, ("ffs_use_bread: non-NULL *bufp %p\n", *bufp)); *bufp = malloc(size, M_UFSMNT, M_WAITOK); if ((error = bread((struct vnode *)devfd, btodb(loc), size, NOCRED, - &bp)) != 0) { - free(*bufp, M_UFSMNT); - *bufp = NULL; + &bp)) != 0) return (error); - } bcopy(bp->b_data, *bufp, size); bp->b_flags |= B_INVAL | B_NOCACHE; brelse(bp);