[libvirt] [PATCH]: don't harcode buffer for getgrnam_r
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply? Cheers, -- Guido
On Thu, Apr 16, 2009 at 09:19:38AM +0200, Guido Günther wrote:
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply?
It looks a bit weird, using sysconf but 1/ allowing it to fail so doing the 2/ 1024 value and loop on ERANGE , but well if I understand correctly taht's forced by some glibc broken behaviour. My take is that the *= 2 size loop should be bounded to avoid eating all memory on some intermediate not size related error. Can we really get over 2^16 groups on a single node ? That sounds like a relatively safe limit, and if it's over it then sysconf() call should have provided the larger value in the first place. So I would be fine with a version with the loop bounded to 65535, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Apr 16, 2009 at 09:56:27AM +0200, Daniel Veillard wrote:
On Thu, Apr 16, 2009 at 09:19:38AM +0200, Guido Günther wrote:
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply?
It looks a bit weird, using sysconf but 1/ allowing it to fail so doing the 2/ 1024 value and loop on ERANGE , but well if I understand correctly taht's forced by some glibc broken behaviour. Yes, sysconf is allowed to return -1 here.
My take is that the *= 2 size loop should be bounded to avoid eating all memory on some intermediate not size related error. Can we really glibc shouldn't return ERANGE then, but better safe than sorry. I've added that check in the attched patch. Cheers, -- Guido
On Thu, Apr 16, 2009 at 02:14:13PM +0200, Guido G?nther wrote:
On Thu, Apr 16, 2009 at 09:56:27AM +0200, Daniel Veillard wrote:
On Thu, Apr 16, 2009 at 09:19:38AM +0200, Guido Günther wrote:
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply?
It looks a bit weird, using sysconf but 1/ allowing it to fail so doing the 2/ 1024 value and loop on ERANGE , but well if I understand correctly taht's forced by some glibc broken behaviour. Yes, sysconf is allowed to return -1 here.
My take is that the *= 2 size loop should be bounded to avoid eating all memory on some intermediate not size related error. Can we really glibc shouldn't return ERANGE then, but better safe than sorry. I've added that check in the attched patch.
ACK. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hi, On Thu, Apr 16, 2009 at 10:40 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Apr 16, 2009 at 02:14:13PM +0200, Guido G?nther wrote:
On Thu, Apr 16, 2009 at 09:56:27AM +0200, Daniel Veillard wrote:
On Thu, Apr 16, 2009 at 09:19:38AM +0200, Guido Günther wrote:
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply?
It looks a bit weird, using sysconf but 1/ allowing it to fail so doing the 2/ 1024 value and loop on ERANGE , but well if I understand correctly taht's forced by some glibc broken behaviour. Yes, sysconf is allowed to return -1 here.
My take is that the *= 2 size loop should be bounded to avoid eating all memory on some intermediate not size related error. Can we really glibc shouldn't return ERANGE then, but better safe than sorry. I've added that check in the attched patch.
ACK.
I think it's more useful that such a function is implemented as a wrapper function like virGetUserDirectory() in util.c, because other drivers might use the function. Actually my patch sent just now implements virGetGIDByGroupname() in util.c ;-) Thanks, ozaki-r
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 16, 2009 at 11:37:24PM +0900, Ryota Ozaki wrote:
Hi,
On Thu, Apr 16, 2009 at 10:40 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Apr 16, 2009 at 02:14:13PM +0200, Guido G?nther wrote:
On Thu, Apr 16, 2009 at 09:56:27AM +0200, Daniel Veillard wrote:
On Thu, Apr 16, 2009 at 09:19:38AM +0200, Guido Günther wrote:
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply?
It looks a bit weird, using sysconf but 1/ allowing it to fail so doing the 2/ 1024 value and loop on ERANGE , but well if I understand correctly taht's forced by some glibc broken behaviour. Yes, sysconf is allowed to return -1 here.
My take is that the *= 2 size loop should be bounded to avoid eating all memory on some intermediate not size related error. Can we really glibc shouldn't return ERANGE then, but better safe than sorry. I've added that check in the attched patch.
ACK.
I think it's more useful that such a function is implemented as a wrapper function like virGetUserDirectory() in util.c, because other drivers might use the function. Actually my patch sent just now implements virGetGIDByGroupname() in util.c ;-)
Looks like we should merge the impls of these two then - put the extra error checking of Guido's patch into this separate virGetGIDByGroupname() helper Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Apr 16, 2009 at 11:37:24PM +0900, Ryota Ozaki wrote:
Hi,
On Thu, Apr 16, 2009 at 10:40 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Apr 16, 2009 at 02:14:13PM +0200, Guido G?nther wrote:
On Thu, Apr 16, 2009 at 09:56:27AM +0200, Daniel Veillard wrote:
On Thu, Apr 16, 2009 at 09:19:38AM +0200, Guido Günther wrote:
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply?
It looks a bit weird, using sysconf but 1/ allowing it to fail so doing the 2/ 1024 value and loop on ERANGE , but well if I understand correctly taht's forced by some glibc broken behaviour. Yes, sysconf is allowed to return -1 here.
My take is that the *= 2 size loop should be bounded to avoid eating all memory on some intermediate not size related error. Can we really glibc shouldn't return ERANGE then, but better safe than sorry. I've added that check in the attched patch.
ACK.
I think it's more useful that such a function is implemented as a wrapper function like virGetUserDirectory() in util.c, because other drivers might use the function. Actually my patch sent just now implements virGetGIDByGroupname() in util.c ;-) Yes, it's more useful to have this in util.c when we have more than once caller (which we hadn't so far). I'll move remoteReadConfigFile over to virGetGIDByGroupname once it's in. -- Guido
On Thu, Apr 16, 2009 at 02:40:44PM +0100, Daniel P. Berrange wrote:
On Thu, Apr 16, 2009 at 02:14:13PM +0200, Guido G?nther wrote:
On Thu, Apr 16, 2009 at 09:56:27AM +0200, Daniel Veillard wrote:
On Thu, Apr 16, 2009 at 09:19:38AM +0200, Guido Günther wrote:
Hi, determines the maximum needed buffersize for getgrnam_r using sysconf instead of hardcoding it to 1024 and increases the buffer on ERANGE. The latter is needed since sysconf is allowed to return -1. Furthermore some glibc versions seem to return a too small buffer on amd64 (http://bugs.debian.org/520744). O.k. to apply?
It looks a bit weird, using sysconf but 1/ allowing it to fail so doing the 2/ 1024 value and loop on ERANGE , but well if I understand correctly taht's forced by some glibc broken behaviour. Yes, sysconf is allowed to return -1 here.
My take is that the *= 2 size loop should be bounded to avoid eating all memory on some intermediate not size related error. Can we really glibc shouldn't return ERANGE then, but better safe than sorry. I've added that check in the attched patch.
ACK. Applied now. -- Guido
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Guido Günther -
Ryota Ozaki