Skip to content

RFC: Introduce pam_alloc.c for unnamed data#767

Draft
stoeckmann wants to merge 2 commits intolinux-pam:masterfrom
stoeckmann:alloc
Draft

RFC: Introduce pam_alloc.c for unnamed data#767
stoeckmann wants to merge 2 commits intolinux-pam:masterfrom
stoeckmann:alloc

Conversation

@stoeckmann
Copy link
Copy Markdown
Contributor

@stoeckmann stoeckmann commented Feb 13, 2024

Currently, the modutil functions like pam_modutil_getpwnam add data through pam_set_data if they have to manually handle allocations to support reentrant functions. This has one drawback:

  • For pam_set_data a name has to be created. Later lookups of other names will iterate over these entries, which will slow down the search algorithm through the linked list.

By adding _pam_add_alloc as well as _pam_free_alloc it is possible to add "unnamed data" to the pam handle. Such data cannot be looked up again and will be freed when pam_end is called.

Such mechanism can be used outside of pam_modutil_getpwnam as well, for example in _pam_mkargv to separate the char pointers from the chars. Which in turn allows us to remove the last strcpy call from libpam.

The functions pam_add_alloc and pam_free_alloc manage unnamed data which
has to be freed when pam_end is called.

Use pam_alloc for data which is never looked up again but has to remain
in heap for later uses. This can be a good foundation for data
management of reentrant function implementations.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Comment thread libpam/pam_modutil_getpwnam.c Outdated
free(buffer);
return NULL;
D(("success"));
return result;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember where does the pamh == NULL case come from and whether it's used anywhere, but if it's used, then there is no way for buffer to be freed in that case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed... Thanks for pointing out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pamh == NULL case would also have to check if reentrant functions exist or not to be sure if the returned memory has to be freed.

Regardless, I've removed the memory split. Let's not make it more complicated.

The allocated memory can be stored without a name, simplifying the code.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Comment on lines +20 to 24
extern int
_pam_add_alloc(pam_handle_t *pamh, void *data);

extern void
pam_modutil_cleanup(pam_handle_t *pamh, void *data,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming convention for this new interface is slightly confusing.
While all other pam_modutil functions have pam_modutil_ prefix, this one has _pam_ prefix instead.

Also, _pam_add_alloc was declared in two different header files, we don't normally do that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to use it not just for modules or modutil releated functionality, but outside of it as well, e.g. for _pam_mkargv. If there is a better way to define it so it's visible from modutil* and libpam itself, without exposing it to everyone else, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants