Bug 1711210

Created attachment 1569971 [details]
Reproducer script

Note: the script exercises the leaks numerous times, and it's really not necessary to leak more than once in order to demonstrate whether the leak has been fixed or not.

I believe it should be sufficient to run the exportfs with 'fsid=...' once


Additional leaks:

        # exportfs -v -i -omp=/some/longer/path \*:/mnt

which leaks (strlen(mp) + 1) bytes from this allocation of ep->e_mountpoint:

                } else if (strcmp(opt, "mountpoint")==0 ||
                           strcmp(opt, "mp") == 0 ||
                           strncmp(opt, "mountpoint=", 11)==0 ||
                           strncmp(opt, "mp=", 3) == 0) {
                        char * mp = strchr(opt, '=');
                        if (mp)
                                ep->e_mountpoint = strdup(mp+1);  <<<<<<<<

==00:00:00:24.636 10815== 18 bytes in 1 blocks are definitely lost in loss record 14 of 37
==00:00:00:24.636 10815==    at 0x4C29BEB: malloc+114 (vg_replace_malloc.c:299)
==00:00:00:24.636 10815==    by 0x573B0C9: strdup+25 (strdup.c:42)
==00:00:00:24.636 10815==    by 0x11730F: parseopts.isra.3+2895 (exports.c:649)
==00:00:00:24.636 10815==    by 0x11787C: getexportent+476 (exports.c:182)
==00:00:00:24.636 10815==    by 0x115049: xtab_read+89 (xtab.c:44)
==00:00:00:24.636 10815==    by 0x10E54B: auth_reload+155 (auth.c:107)
==00:00:00:24.636 10815==    by 0x10E65F: auth_init+15 (auth.c:51)
==00:00:00:24.636 10815==    by 0x10D01D: main+1821 (mountd.c:902)
==00:00:00:24.636 10815==    by 0x56D13D4: __libc_start_main+244 (libc-start.c:266)
==00:00:00:24.636 10815==    by 0x10D3CA: ??? (in /root/rpmbuild/BUILD/nfs-utils-1.3.0/utils/mountd/mountd)


and:
        # exportfs -v -i -omp \*:/mnt
which leaks 1 byte from the allocation in the 'else' clause:
                        if (mp)
                                ep->e_mountpoint = strdup(mp+1);
                        else
                                ep->e_mountpoint = strdup(""); <<<<<<<<

==00:00:00:19.838 9975== 1 bytes in 1 blocks are definitely lost in loss record 2 of 34
==00:00:00:19.838 9975==    at 0x4C2BB46: calloc+138 (vg_replace_malloc.c:711)
==00:00:00:19.838 9975==    by 0x1174EF: parseopts.isra.3+3375 (exports.c:651)
==00:00:00:19.838 9975==    by 0x11787C: getexportent+476 (exports.c:182)
==00:00:00:19.838 9975==    by 0x115049: xtab_read+89 (xtab.c:44)
==00:00:00:19.838 9975==    by 0x10E54B: auth_reload+155 (auth.c:107)
==00:00:00:19.838 9975==    by 0x10E65F: auth_init+15 (auth.c:51)
==00:00:00:19.838 9975==    by 0x10D01D: main+1821 (mountd.c:902)
==00:00:00:19.838 9975==    by 0x56D13D4: __libc_start_main+244 (libc-start.c:266)
==00:00:00:19.838 9975==    by 0x10D3CA: ??? (in /root/rpmbuild/BUILD/nfs-utils-1.3.0/utils/mountd/mountd)


        # exportfs -v -i -orefer=/some_path \*:/mnt
which leaks (strlen(refer) + 1) from this allocation:
                } else if (strncmp(opt, "refer=", 6) == 0) {
                        ep->e_fslocmethod = FSLOC_REFER;
                        ep->e_fslocdata = strdup(opt+6);  <<<<<<<<

==00:00:00:46.441 12188== 11 bytes in 1 blocks are definitely lost in loss record 6 of 35
==00:00:00:46.441 12188==    at 0x4C29BEB: malloc+114 (vg_replace_malloc.c:299)
==00:00:00:46.441 12188==    by 0x573B0C9: strdup+25 (strdup.c:42)
==00:00:00:46.441 12188==    by 0x1174D4: parseopts.isra.3+3348 (exports.c:664)
==00:00:00:46.441 12188==    by 0x11787C: getexportent+476 (exports.c:182)
==00:00:00:46.441 12188==    by 0x115049: xtab_read+89 (xtab.c:44)
==00:00:00:46.441 12188==    by 0x10E54B: auth_reload+155 (auth.c:107)
==00:00:00:46.441 12188==    by 0x10E65F: auth_init+15 (auth.c:51)
==00:00:00:46.441 12188==    by 0x10D01D: main+1821 (mountd.c:902)

Agreed, e_fslocdata and e_mountpoint leak too. 

If e_squids and e_sqgids are leaking too, might make more sense simply calling exportent_release() to free up everything, coz we have that function in export.c, 

void
exportent_release(struct exportent *eep)
{
        xfree(eep->e_squids);
        xfree(eep->e_sqgids);
        free(eep->e_mountpoint);
        free(eep->e_fslocdata);
        free(eep->e_uuid);
        xfree(eep->e_hostname);
}

(and maybe change exportent_release to check for NULL before freeing and then setting it to null)


As of now, the patch we are planning to submit upstream for hostname and uuid fixes looks like, (for steve's reference),


static int
xtab_read(char *xtab, char *lockfn, int is_export)
{

    /* is_export == 0  => reading /proc/fs/nfs/exports - we know these things are exported to kernel
     * is_export == 1  => reading /var/lib/nfs/etab - these things are allowed to be exported
     */
        struct exportent        *xp;
        nfs_export              *exp;
        int                     lockid;

        if ((lockid = xflock(lockfn, "r")) < 0)
                return 0;
        setexportent(xtab, "r");
        if (is_export == 1)
                v4root_needed = 1;
        while ((xp = getexportent(is_export==0, 0)) != NULL) {
                if (!(exp = export_lookup(xp->e_hostname, xp->e_path, is_export != 1)) &&
                    !(exp = export_create(xp, is_export!=1)))
                {
                    if(xp->e_hostname) {
                        free(xp->e_hostname);
                        xp->e_hostname=NULL;
                    }
                    if(xp->e_uuid) {
                        free(xp->e_uuid);
                        xp->e_uuid=NULL;
                    }

                    continue;
                }
                switch (is_export) {
                case 0:
                        exp->m_exported = 1;
                        break;
                case 1:
                        exp->m_xtabent = 1;
                        exp->m_mayexport = 1;
                        if ((xp->e_flags & NFSEXP_FSID) && xp->e_fsid == 0)
                                v4root_needed = 0;
                        break;
                }
                if(xp->e_hostname) {
                    free(xp->e_hostname);
                    xp->e_hostname=NULL;
                }
                if(xp->e_uuid) {
                    free(xp->e_uuid);
                    xp->e_uuid=NULL;
                }


        }
        endexportent();
        xfunlock(lockid);

        return 0;
}


And in exports.c , the last part of getexportent() is,

...
...
        ee.e_hostname = xstrdup(hostname);

        if (parseopts(opt, &ee, fromexports && !has_default_subtree_opts, NULL) < 0) {
                if(ee.e_hostname)
                {
                    xfree(ee.e_hostname);
                    ee.e_hostname=NULL;
                }
                if(ee.e_uuid)
                {
                    xfree(ee.e_uuid);
                    ee.e_uuid=NULL;
                }
                return NULL;
        }
        /* resolve symlinks */
        if (realpath(ee.e_path, rpath) != NULL) {
                rpath[sizeof (rpath) - 1] = '\0';
                strncpy(ee.e_path, rpath, sizeof (ee.e_path) - 1);
                ee.e_path[sizeof (ee.e_path) - 1] = '\0';
        }
        return &ee;
}


This seems to work to at least fix the hostname and uuid ones.

The upstream commit

commit 50ef80739d9e1e0df6616289ef2ff626a94666ee
Author: Steve Dickson <steved>
Date:   Thu May 23 09:24:49 2019 -0400

    rpc.mountd: Fix e_hostname and e_uuid leaks
    
    strdup of exportent uuid and hostname in getexportent() ends up leaking
    memory. Free the memory before getexportent() is called again from xtab_read()

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2019:2268