acl_from_text leaking memory

[Jim Wilcoxson]

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
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hacke...@freebsd.org"

[Gary Jennejohn]

On Sun, 15 Nov 2009 11:47:28 -0500
Jim Wilcoxson <pri...@gmail.com> 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:
>

[snip code]

Looks to me like you're right.

Have you tried applying the suggested change and testing it for
regressions?

Reporting that it works without side effects would definitely be
convincing and increase the chances of getting it committed.

---
Gary Jennejohn

[vol...@vwsoft.com]

Jim,

you may want to have a look at the manpage acl_from_text(3):

"...This function may cause memory to be allocated. The caller should
free any releasable memory, when the new ACL is no longer required, by
calling acl_free(3) with the (void *)acl_t as an argument."

Please use an acl_free(void *obj_p) call afterwards to avoid leaking memory.

Volker

[Gary Jennejohn]

On Mon, 16 Nov 2009 21:12:47 +0100
vol...@vwsoft.com wrote:

> you may want to have a look at the manpage acl_from_text(3):
>
> "...This function may cause memory to be allocated. The caller should
> free any releasable memory, when the new ACL is no longer required, by
> calling acl_free(3) with the (void *)acl_t as an argument."
>
> Please use an acl_free(void *obj_p) call afterwards to avoid leaking memory.
>

The suggested fix was appplied to HEAD today. Apparently, the man page should
now be updated.

---
Gary Jennejohn

[Jim Wilcoxson]

The man page is correct and should not be changed.

In the example program I submitted, it does call acl_free; this is not
where the leak occurs. The leak occurs because of a temporary string
that acl_from_text allocates to parse the text.

Jim

[vol...@vwsoft.com]

On 11/16/09 22:21, Jim Wilcoxson wrote:
> The man page is correct and should not be changed.
>
> In the example program I submitted, it does call acl_free; this is not
> where the leak occurs. The leak occurs because of a temporary string
> that acl_from_text allocates to parse the text.
>
> Jim
>
> On 11/16/09, Gary Jennejohn <gary.je...@freenet.de> wrote:
>> On Mon, 16 Nov 2009 21:12:47 +0100
>> vol...@vwsoft.com wrote:
>>
>>> you may want to have a look at the manpage acl_from_text(3):
>>>
>>> "...This function may cause memory to be allocated. The caller should
>>> free any releasable memory, when the new ACL is no longer required, by
>>> calling acl_free(3) with the (void *)acl_t as an argument."
>>>
>>> Please use an acl_free(void *obj_p) call afterwards to avoid leaking
>>> memory.
>>>
>> The suggested fix was appplied to HEAD today. Apparently, the man page
>> should
>> now be updated.
>>
>> ---
>> Gary Jennejohn
>>
>

Yes, I see and c199317 fixed that leak correctly. Jim is right - the
manpage still should not be changed as the caller is still responsible
for free'ing allocated memory.

Volker