Page MenuHomeFreeBSD

Fix a memory leak in pw when invoked with -V or -R
Needs ReviewPublic

Authored by momchil_xaxo.eu on Apr 6 2019, 9:15 PM.

Details

Summary

pw leaks memory when invoked with the -V or -R arguments. The following example leaks memory

pw -V /etc usershow root

This does not leak any memory

pw usershow root

The memory leak is more prominent when

pw -V /etc usershow -a

is invoked as a portion of memory is leaked for each entry in the user table.

The passwd and group structures are allocated with malloc() in vnextpwent() and vnextgrent() when the -V or -R options are used. These call pw_scan() and gr_scan() which call pw_dup() and gr_dup(). The latter calls gr_add() which does calls malloc(). Calls to GETPWENT(), GETPWNAM(), GETGRENT() and GETGRNAM() are used to obtain the structures. These structures are not later released via free(). The missing calls to free() are probably due the fact that they are not necessary when pw is not invoked with either -V or -R arguments. In this case the functions vnextpwent() and vnextgrent() are not used. Instead, the functions getpwent() and getgrent() are used. These do not require the user to free the structures.

The proposed solution: record the last structures provided by vnextpwent() and vnextgrent() and free them upon consecutive calls. This solution is in the spirit of getpwent() and getgrent().

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

momchil_xaxo.eu created this revision.Apr 6 2019, 9:15 PM
momchil_xaxo.eu retitled this revision from ix a memory leak in pw when invoked with -V or -R to Fix a memory leak in pw when invoked with -V or -R.Apr 6 2019, 9:16 PM

Call free() upon normal program termination as well.

An even better solution would be to call free() before every call to errx() in pw_vpw.c, but that seems like a tedious task as there are many errx() calls in pw_vpw.c and errx() does not return anyway. I don't know what the policy on calling errx() is, therefore that bit is omitted.

Last, in main(), call free() only when the pointers are not null.

It's not clear to me how this leaks memory. Can you go into a bit more detail about how you found the memory leak, and the codepath it follows?

pw.c
209

free(NULL); is valid, so there's no need to check for NULL first.

When I execute this

valgrind pw -V /etc/ usershow -a

on my system

11.2-RELEASE-p5 FreeBSD 11.2-RELEASE-p5 #0: Tue Nov 27 09:33:52 UTC 2018 root@amd64-builder.daemonology.net:/usr/obj/usr/src/sys/GENERIC amd64

I get the following result

53930

53930== HEAP SUMMARY:

53930== in use at exit: 14,593 bytes in 80 blocks

53930== total heap usage: 2,992 allocs, 2,912 frees, 1,400,218 bytes allocated

53930

53930== LEAK SUMMARY:

53930== definitely lost: 6,991 bytes in 74 blocks

53930== indirectly lost: 0 bytes in 0 blocks

53930== possibly lost: 0 bytes in 0 blocks

53930== still reachable: 7,602 bytes in 6 blocks

53930== suppressed: 0 bytes in 0 blocks

I tried it now in a virtual machine with

12.0-RELEASE FreeBSD 12.0-RELEASE r341666 GENERIC amd64

and got no leak. Maybe this has been fixed inbetween?

The logic that I have followed is the following:

  • pw uses the following macros from pwupd.h to scan the users/groups

#define SETPWENT() PWF._setpwent()
#define ENDPWENT() PWF._endpwent()
#define GETPWENT() PWF._getpwent()
#define GETPWUID(uid) PWF._getpwuid(uid)
#define GETPWNAM(nam) PWF._getpwnam(nam)

#define SETGRENT() PWF._setgrent()
#define ENDGRENT() PWF._endgrent()
#define GETGRENT() PWF._getgrent()
#define GETGRGID(gid) PWF._getgrgid(gid)
#define GETGRNAM(nam) PWF._getgrnam(nam)

  • there are two structures in pw.c

struct pwf PWF =
{
PWF_REGULAR,
setpwent,
endpwent,
getpwent,
getpwuid,
getpwnam,
setgrent,
endgrent,
getgrent,
getgrgid,
getgrnam,

};
struct pwf VPWF =
{
PWF_ALT,
vsetpwent,
vendpwent,
vgetpwent,
vgetpwuid,
vgetpwnam,
vsetgrent,
vendgrent,
vgetgrent,
vgetgrgid,
vgetgrnam,
};

  • the PWF structure is replaced in pw.c with VPWF in case -V or -R are given as arguments
				memcpy(&PWF, &VPWF, sizeof PWF);
  • man getpwent(3) says

BUGS

The functions getpwent(), getpwnam(), and getpwuid(), leave their results
in an internal static object and return a pointer to that object.
Subsequent calls to the same function will modify the same object.
  • therefore in case you use these functions, there is no need to free() the obtained pointer. This is the case when the original PWF table is used.
  • The VPWF table, however, uses functions that scan the passwd and group files from any directory. These are located in pw_vpw.c. Take for example the vnextpwent() function. It calls pw_scan() from libutil for each line which is not empty or is not commented. The returned value of pw_scan() is returned to the caller if everything is ok. Now pw_scan() calls pw_dup() which provides a copy of the password structure. This copy is returned by pw_scan(), therefore it is returned by vnextpwent(). The function pw_dup() allocates the necessary memory with malloc(). This function is located in lib/libutil/pw_util.c. I do not seem to find the place where this portion of memory is deallocated with free() after it has been used. See pw_user.c for instance, look for uses of GETPWENT(). Here is an example from pw_user_show():

if (all) {

		SETPWENT();
		while ((pwd = GETPWENT()) != NULL)
			print_user(pwd, pretty, v7);
		ENDPWENT();
		return (EXIT_SUCCESS);

}

  • Maybe I am looking at older code or I am missing something?
  • The check for NULL I can remove from the patch.

cc bapt who knows more about this code than I do.

bapt added a comment.Apr 18 2019, 11:28 AM

I haven't been through all the code yet, but my understanding is that the leak has been "fixed" when libutil was modified, see rS336746

My understanding is that [1] fixes only occasional segmentation faults in pw_scan() by adding

__pw_initpwd(&pw);

on line 673. Without this strlen() segfaults in pw_dup(), called by pw_scan() on line 678

ret = pw_dup(&pw);

since the fields in the pw structure, on which strlen() is called, are not initialised at all and can point to garbage. I see no code there to keep track of the allocated memory, nor any code to free() that allocated memory. My understanding is that the caller of pw_scan() has the responsibility of deallocating that memory with free().

1: https://reviews.freebsd.org/rS336746