* [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]
@ 2018-06-25 17:49 Florian Weimer
2018-06-25 20:37 ` DJ Delorie
0 siblings, 1 reply; 4+ messages in thread
From: Florian Weimer @ 2018-06-25 17:49 UTC (permalink / raw)
To: libc-alpha
Remove extend_alloca usage. Also adjusts the internal function get_uid.
2018-06-25 Florian Weimer <fweimer@redhat.com>
[BZ #18023]
* nis/nss_nis/nis-initgroups.c (get_uid, _nss_nis_initgroups_dyn):
Use struct scratch_buffer instead of extend_alloca.
diff --git a/nis/nss_nis/nis-initgroups.c b/nis/nss_nis/nis-initgroups.c
index a47b4d7ada..ca043d4e5a 100644
--- a/nis/nss_nis/nis-initgroups.c
+++ b/nis/nss_nis/nis-initgroups.c
@@ -16,7 +16,6 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include <alloca.h>
#include <ctype.h>
#include <errno.h>
#include <grp.h>
@@ -27,6 +26,7 @@
#include <rpcsvc/yp.h>
#include <rpcsvc/ypclnt.h>
#include <sys/param.h>
+#include <scratch_buffer.h>
#include "nss-nis.h"
#include <libnsl.h>
@@ -120,27 +120,30 @@ internal_getgrent_r (struct group *grp, char *buffer, size_t buflen,
static int
get_uid (const char *user, uid_t *uidp)
{
- size_t buflen = sysconf (_SC_GETPW_R_SIZE_MAX);
- char *buf = (char *) alloca (buflen);
+ struct scratch_buffer tmpbuf;
+ scratch_buffer_init (&tmpbuf);
while (1)
{
struct passwd result;
struct passwd *resp;
- int r = getpwnam_r (user, &result, buf, buflen, &resp);
+ int r = getpwnam_r (user, &result, tmpbuf.data, tmpbuf.length, &resp);
if (r == 0 && resp != NULL)
{
*uidp = resp->pw_uid;
+ scratch_buffer_free (&tmpbuf);
return 0;
}
if (r != ERANGE)
break;
- buf = extend_alloca (buf, buflen, 2 * buflen);
+ if (!scratch_buffer_grow (&tmpbuf))
+ return 1;
}
+ scratch_buffer_free (&tmpbuf);
return 1;
}
@@ -254,8 +257,6 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start,
}
struct group grpbuf, *g;
- size_t buflen = sysconf (_SC_GETPW_R_SIZE_MAX);
- char *tmpbuf;
enum nss_status status;
intern_t intern = { NULL, NULL, 0 };
gid_t *groups = *groupsp;
@@ -264,15 +265,20 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start,
if (status != NSS_STATUS_SUCCESS)
return status;
- tmpbuf = __alloca (buflen);
+ struct scratch_buffer tmpbuf;
+ scratch_buffer_init (&tmpbuf);
while (1)
{
while ((status =
- internal_getgrent_r (&grpbuf, tmpbuf, buflen, errnop,
+ internal_getgrent_r (&grpbuf, tmpbuf.data, tmpbuf.length, errnop,
&intern)) == NSS_STATUS_TRYAGAIN
&& *errnop == ERANGE)
- tmpbuf = extend_alloca (tmpbuf, buflen, 2 * buflen);
+ if (!scratch_buffer_grow (&tmpbuf))
+ {
+ status = NSS_STATUS_TRYAGAIN;
+ goto done;
+ }
if (status != NSS_STATUS_SUCCESS)
{
@@ -331,6 +337,7 @@ done:
intern.start = intern.start->next;
free (intern.next);
}
+ scratch_buffer_free (&tmpbuf);
return status;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]
2018-06-25 17:49 [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023] Florian Weimer
@ 2018-06-25 20:37 ` DJ Delorie
2018-06-25 20:43 ` Florian Weimer
0 siblings, 1 reply; 4+ messages in thread
From: DJ Delorie @ 2018-06-25 20:37 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
fweimer@redhat.com (Florian Weimer) writes:
> - buf = extend_alloca (buf, buflen, 2 * buflen);
> + if (!scratch_buffer_grow (&tmpbuf))
> + return 1;
The docs say if _grow() fails, the buffer is still free'able. Could we
not, or should we, use "break" here so that scratch_buffer_free() is
called when grow() fails?
The docs (scratch_buffer.h) mentions that the old buffer is freed, but
that the scratch_buffer is still "in a freeable state" so it's not clear
if the intention is that you *should* call scratch_buffer_free(), or
*must*, or *must not*, etc.
> }
>
> + scratch_buffer_free (&tmpbuf);
> return 1;
> }
> + if (!scratch_buffer_grow (&tmpbuf))
> + {
> + status = NSS_STATUS_TRYAGAIN;
> + goto done;
> + }
In this case, going to "done" *does* call scratch_buffer_free(). At
least we should be consistent within a given file ;-)
> if (status != NSS_STATUS_SUCCESS)
> {
> @@ -331,6 +337,7 @@ done:
> intern.start = intern.start->next;
> free (intern.next);
> }
> + scratch_buffer_free (&tmpbuf);
>
> return status;
> }
The code itself looks OK, the above are just consistency and
clarification things.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]
2018-06-25 20:37 ` DJ Delorie
@ 2018-06-25 20:43 ` Florian Weimer
2018-06-25 21:46 ` DJ Delorie
0 siblings, 1 reply; 4+ messages in thread
From: Florian Weimer @ 2018-06-25 20:43 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
On 06/25/2018 10:37 PM, DJ Delorie wrote:
>
> fweimer@redhat.com (Florian Weimer) writes:
>> - buf = extend_alloca (buf, buflen, 2 * buflen);
>> + if (!scratch_buffer_grow (&tmpbuf))
>> + return 1;
>
> The docs say if _grow() fails, the buffer is still free'able. Could we
> not, or should we, use "break" here so that scratch_buffer_free() is
> called when grow() fails?
>
> The docs (scratch_buffer.h) mentions that the old buffer is freed, but
> that the scratch_buffer is still "in a freeable state" so it's not clear
> if the intention is that you *should* call scratch_buffer_free(), or
> *must*, or *must not*, etc.
You can free the buffer again, but you don't have to. There is no
memory leak if you don't.
>
>> }
>>
>> + scratch_buffer_free (&tmpbuf);
>> return 1;
>> }
The point here is not call free if we know it is a no-op.
So I think the code is okay as-is.
>> + if (!scratch_buffer_grow (&tmpbuf))
>> + {
>> + status = NSS_STATUS_TRYAGAIN;
>> + goto done;
>> + }
>
> In this case, going to "done" *does* call scratch_buffer_free(). At
> least we should be consistent within a given file ;-)
Here we call the entire cleanup sequence, which happens to include
scratch_buffer_free as well, so the situation is different.
I hope this clarifies why things are the way they are.
Thanks,
Florian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]
2018-06-25 20:43 ` Florian Weimer
@ 2018-06-25 21:46 ` DJ Delorie
0 siblings, 0 replies; 4+ messages in thread
From: DJ Delorie @ 2018-06-25 21:46 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
Florian Weimer <fweimer@redhat.com> writes:
> You can free the buffer again, but you don't have to. There is no
> memory leak if you don't.
>
> The point here is not call free if we know it is a no-op.
>
> So I think the code is okay as-is.
Agreed, just checking ;-)
>> In this case, going to "done" *does* call scratch_buffer_free(). At
>> least we should be consistent within a given file ;-)
>
> Here we call the entire cleanup sequence, which happens to include
> scratch_buffer_free as well, so the situation is different.
Ok, as long as the inconsistency was intentional and reasoned, I'm OK
with it.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-25 21:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-25 17:49 [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023] Florian Weimer
2018-06-25 20:37 ` DJ Delorie
2018-06-25 20:43 ` Florian Weimer
2018-06-25 21:46 ` DJ Delorie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).