On Sat, Feb 18, 2012 at 09:28:14AM -0500, Simo Sorce wrote:
On Sat, 2012-02-18 at 12:55 +0100, Jakub Hrozek wrote:
> On Sat, Feb 18, 2012 at 10:15:47AM +0100, Jakub Hrozek wrote:
> > Provides a static storage that keeps track of getgr[nam|gid] results
> in
> > case the buffer supplied by caller is not big enough.
> >
> >
https://fedorahosted.org/sssd/ticket/1191
> >
>
> Self-nack, the original patch was leaking memory. A new patch is
> attached.
>
NACK,
see inline
> +static struct sss_nss_getgr_data {
> + union {
> + char *grname;
> + gid_t gid;
> + } id;
> + uint8_t *repbuf;
> + size_t replen;
> +} sss_nss_getgr_data;
You should add a type here, as is this structure is unsafe.
Add:
enum data_type = {
GETGR_NONE,
GETGR_NAME,
GETGR_GID
} type;
Thanks, this is fixed.
> +static void sss_nss_getgr_data_clean(void)
> +{
> + if (sss_nss_getgr_data.repbuf) {
> + free(sss_nss_getgr_data.repbuf);
> + sss_nss_getgr_data.repbuf = NULL;
> + }
> + sss_nss_getgr_data.replen = 0;
> +}
> +
> +static void sss_nss_getgrnam_data_clean(void)
> +{
> + if (sss_nss_getgr_data.id.grname) {
> + free(sss_nss_getgr_data.id.grname);
> + sss_nss_getgr_data.id.grname = NULL;
> + }
> + sss_nss_getgr_data_clean();
> +}
> +
> +static void sss_nss_getgrgid_data_clean(void)
> +{
> + sss_nss_getgr_data.id.grname = 0;
> + sss_nss_getgr_data_clean();
> +}
Please collapse the above 3 functions into a single one that act based
on the type defined above.
> +static void sss_nss_getgr_get(uint8_t **repbuf, size_t *replen)
> +{
> + *repbuf = sss_nss_getgr_data.repbuf;
> + *replen = sss_nss_getgr_data.replen;
> +}
I am honestly not sure it is worth to have a getter, when all you do is
assign 2 buffers (same comment for the setter.
Two or more, use a for :-) I was trying to avoid code duplucation, but
perhaps that was too much. The atached patch collapsed the functions
into a single one.
> +static errno_t sss_nss_getgrnam_again(const char *name,
uint8_t
> **repbuf,
> + size_t *replen)
> +{
> + if (sss_nss_getgr_data.id.grname &&
> + strcmp(sss_nss_getgr_data.id.grname, name) == 0) {
This is wrong, you do not know what was stored before it cuold have been
a gid. Without storing and then checking the type, you are going to test
a gid value as it was a memory pointer. In most cases this will be a
segfault in the application!!
Fixed (and similar cases elsewhere).
> + sss_nss_lock();
> +
> + fast = false;
> + ret = sss_nss_getgrnam_again(name, &repbuf, &replen);
> + if (ret == EOK) {
> + fast = true;
> + FILL_GRREP(grrep, result, buffer, buflen);
> + goto reply;
This got is a bad idea, makes reviewing this code diffcult, and you are
not chasing an error condition.
Use gotos only as exceptions, any other use is not acceptable.
Actually I think this case should be acceptable - it just shortcuts into
one place in one precisely defined condition.
The attached patched switched to not using goto also because it memcpys
the buffer as opposed to just keeping the buffer. This approach also
simplifies error handling.
> +reply:
> len = replen - 8;
> ret = sss_nss_getgr_readrep(&grrep, repbuf+8, &len);
> - free(repbuf);
> + if (ret == ERANGE) {
> + /* The buffer glibc supplied us with was too small. Let's keep
> + * the results in memory if glibc retries with a bigger buffer
> */
> + if (sss_nss_getgrnam_save(name, name_len, repbuf, replen) !=
> EOK) {
> + /* Not fatal, we'll just go to SSSD next time */
> + free(repbuf);
> + }
> + } else {
> + if (fast) {
> + sss_nss_getgrnam_data_clean();
> + } else {
> + free(repbuf);
> + }
> + }
Bundle the above up into a function, remove fast and handle cleanup
where you now goto. Remember to unlock.
The cleanup is now simplified due to coping the buffer. There's only one
exit from the function which is after the out: label, so unlocking is
performed there.
Thank you for the review.