[PC-BSD Testing] acl_from_text leaking memory

[doverosx at gmail.com]

Jim Wilcoxson wrote:
> mybuf_p is a temporary copy of the text acl.  The reason a temp is
> created is so that while parsing the acl text with strsep, it can be
> modified by inserting zero bytes.  There are actually 2 mallocs
> happening: one is the temporary text string, mybuf_p; the other is the
> acl itself that will be returned.  You're right - it would not do to
> free the acl itself.  The caller of acl_from_text has to do that with
> acl_free, and the test program I used does this.
>
> The leak is (I think) caused by the temporary copy of the text string:
> it isn't freed after the acl has been parsed.  I would test this by
> making the change and creating a new kernel, but I've never built a
> BSD kernel.
>
> Jim
>
> On 11/15/09, doverosx at gmail.com <doverosx at gmail.com> wrote:
>   
>> Jim Wilcoxson wrote:
>>     
>>> I've been working on a new backup program, HashBackup, and believe I
>>> have found a memory leak with ACLs in PCBSD/FreeBSD 7.1 and OSX
>>> (Leopard).
>>>
>>> acl_from_text is a function that takes a text string as input, and
>>> returns a pointer to a malloc'd acl.  This acl is then freed with
>>> acl_free.  I noticed that acl_from_text appears to leak memory.  This
>>> is not used during the backup of a filesystem, but is needed to do a
>>> restore.
>>>
>>> After looking at the acl_from_text source in /usr/src/lib/libc/posix1e
>>> (from PCBSD7.1), I believe the problem is that the duplicate text
>>> string, mybuf_p, is not freed on normal return of this function.  Here
>>> is the end of this function:
>>>
>>>         }
>>>
>>> #if 0
>>>         /* XXX Should we only return ACLs valid according to acl_valid? */
>>>         /* Verify validity of the ACL we read in. */
>>>         if (acl_valid(acl) == -1) {
>>>                 errno = EINVAL;
>>>                 goto error_label;
>>>         }
>>> #endif
>>>
>>>         return(acl);
>>>
>>> error_label:
>>>         acl_free(acl);
>>>         free(mybuf_p);
>>>         return(NULL);
>>> }
>>>
>>> I think there should be a free(mybuf_p) before return(acl).
>>>
>>> Here is a PCBSD/FreeBSD test program that causes the memory leak:
>>>
>>> #include <stdio.h>
>>> #include <sys/types.h>
>>> #include <sys/acl.h>
>>>
>>> main() {
>>>    acl_t acl;
>>>    char* acltext;
>>>
>>>    acltext = "user::rw-\n group::r--\n mask::r--\n other::r--\n";
>>>    while (1) {
>>>       acl = acl_from_text(acltext);
>>>       if (acl == NULL)
>>>         printf("acl_from_text failed\n");
>>>       if (acl_free(acl) != 0)
>>>         printf("acl_free failed\n");
>>>   }
>>> }
>>>
>>> I've subscribed to the lists for a few days in case there are
>>> questions or I can help test something.
>>>
>>> Thanks,
>>> Jim
>>> --
>>> HashBackup beta: http://sites.google.com/site/hashbackup
>>> _______________________________________________
>>> Testing mailing list
>>> Testing at lists.pcbsd.org
>>> http://lists.pcbsd.org/mailman/listinfo/testing
>>>
>>>
>>>       
>> What is the mybuf_p structure? From what I can interpret in the source,
>> mybuf_p is likely to be kept because our acl is indeed valid. Perhaps
>> mybuf_p should be freed in a more logical function?
>>
>> Regards,
>> Brodey
>> _______________________________________________
>> Testing mailing list
>> Testing at lists.pcbsd.org
>> http://lists.pcbsd.org/mailman/listinfo/testing
>>
>>     
> _______________________________________________
> Testing mailing list
> Testing at lists.pcbsd.org
> http://lists.pcbsd.org/mailman/listinfo/testing
>
>   
Gotcha!

 From my limited scope on the BSD code, I'd agree.

/usr/src/sys/i386 (or amd64)

is the dir for source code.

You can hit up the *BSD hand book but this should work:

replace old .c or .h files in /usr/src/sys/*arch for change*

cd /usr/src/sys

make buildkernel KERNCONF=(GENERIC unless in PC-BSD)

make installkernel KERNCONF=(GENERIC unless in PC-BSD)

Reboot, enjoy!

If you have a FreeBSD machine to test, that'd be what you want.

Later,
Brodey