On Tue, Aug 27, 2013 at 10:42:13AM +0200, Pavel Březina wrote:
I already saw the patch (gave it to a customer in a test build actually), so it's a fast ACK from me.
By the way the bug dates back to 2008, it's one of the first commits in SSSD history :-)
On (27/08/13 10:42), Pavel Březina wrote:
From 573dcd8fc8a87bcfab61d0113c3bc3a49b81f95c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 22 Aug 2013 14:38:54 +0200 Subject: [PATCH] sss_packet_grow: correctly pad packet length to 512B
https://fedorahosted.org/sssd/ticket/2059
If len % SSSSRV_PACKET_MEM_SIZE == 0 or some low number, we can end up with totlen < len and return EINVAL.
It also does not pad the length, but usually allocates much more memory than is desired.
len = 1024 n = 1024 % 512 + 1 = 0 + 1 = 1 totlen = 1 * 512 = 512 => totlen < len
len = 511 n = 511 % 512 + 1 = 511 + 1 totlen = 512 * 512 = 262144 totlen is way bigger than it was supposed to be
src/responder/common/responder_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/responder/common/responder_packet.c b/src/responder/common/responder_packet.c index 5132d955b71eb025cf54a41edd41da779d352f2e..6476bd6e59d6845042ab97bbc1846f9c76f68b57 100644 --- a/src/responder/common/responder_packet.c +++ b/src/responder/common/responder_packet.c @@ -105,7 +105,7 @@ int sss_packet_grow(struct sss_packet *packet, size_t size)
/* make sure we do not overflow */ if (totlen < len) {
int n = len % SSSSRV_PACKET_MEM_SIZE + 1;
int n = len / SSSSRV_PACKET_MEM_SIZE + 1; totlen += n * SSSSRV_PACKET_MEM_SIZE; if (totlen < len) { return EINVAL;
len = 1024 n = 1024 / 512 +1 n = 3 totlen = 3* 512 totlen = 1536
and sufficient length is 1024
what about
int n = (len + SSSSRV_PACKET_MEM_SIZE -1) / SSSSRV_PACKET_MEM_SIZE;
LS
On Tue, 2013-08-27 at 10:54 +0200, Lukas Slebodnik wrote:
On (27/08/13 10:42), Pavel Březina wrote:
From 573dcd8fc8a87bcfab61d0113c3bc3a49b81f95c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 22 Aug 2013 14:38:54 +0200 Subject: [PATCH] sss_packet_grow: correctly pad packet length to 512B
https://fedorahosted.org/sssd/ticket/2059
If len % SSSSRV_PACKET_MEM_SIZE == 0 or some low number, we can end up with totlen < len and return EINVAL.
It also does not pad the length, but usually allocates much more memory than is desired.
len = 1024 n = 1024 % 512 + 1 = 0 + 1 = 1 totlen = 1 * 512 = 512 => totlen < len
len = 511 n = 511 % 512 + 1 = 511 + 1 totlen = 512 * 512 = 262144 totlen is way bigger than it was supposed to be
src/responder/common/responder_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/responder/common/responder_packet.c b/src/responder/common/responder_packet.c index 5132d955b71eb025cf54a41edd41da779d352f2e..6476bd6e59d6845042ab97bbc1846f9c76f68b57 100644 --- a/src/responder/common/responder_packet.c +++ b/src/responder/common/responder_packet.c @@ -105,7 +105,7 @@ int sss_packet_grow(struct sss_packet *packet, size_t size)
/* make sure we do not overflow */ if (totlen < len) {
int n = len % SSSSRV_PACKET_MEM_SIZE + 1;
int n = len / SSSSRV_PACKET_MEM_SIZE + 1; totlen += n * SSSSRV_PACKET_MEM_SIZE; if (totlen < len) { return EINVAL;len = 1024 n = 1024 / 512 +1 n = 3 totlen = 3* 512 totlen = 1536
and sufficient length is 1024
what about
int n = (len + SSSSRV_PACKET_MEM_SIZE -1) / SSSSRV_PACKET_MEM_SIZE;
No we are not aiming at perfect match in case len is a perfect multiple of SSSSRV_PACKET_MEM_SIZE, 1536 is a good answer as it overallocates the buffer and gives us more space to work with.
Overallocating by a full chunk is ok and desirable as it allow the body to grow without yet another allocation. If we wanted perfect buffer lengths we wouldn't allocate in chunks.
Ack to Pavel's patch.
Simo.
On Wed, Aug 28, 2013 at 09:49:39AM -0400, Simo Sorce wrote:
On Tue, 2013-08-27 at 10:54 +0200, Lukas Slebodnik wrote:
On (27/08/13 10:42), Pavel Březina wrote:
From 573dcd8fc8a87bcfab61d0113c3bc3a49b81f95c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 22 Aug 2013 14:38:54 +0200 Subject: [PATCH] sss_packet_grow: correctly pad packet length to 512B
https://fedorahosted.org/sssd/ticket/2059
If len % SSSSRV_PACKET_MEM_SIZE == 0 or some low number, we can end up with totlen < len and return EINVAL.
It also does not pad the length, but usually allocates much more memory than is desired.
len = 1024 n = 1024 % 512 + 1 = 0 + 1 = 1 totlen = 1 * 512 = 512 => totlen < len
len = 511 n = 511 % 512 + 1 = 511 + 1 totlen = 512 * 512 = 262144 totlen is way bigger than it was supposed to be
src/responder/common/responder_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/responder/common/responder_packet.c b/src/responder/common/responder_packet.c index 5132d955b71eb025cf54a41edd41da779d352f2e..6476bd6e59d6845042ab97bbc1846f9c76f68b57 100644 --- a/src/responder/common/responder_packet.c +++ b/src/responder/common/responder_packet.c @@ -105,7 +105,7 @@ int sss_packet_grow(struct sss_packet *packet, size_t size)
/* make sure we do not overflow */ if (totlen < len) {
int n = len % SSSSRV_PACKET_MEM_SIZE + 1;
int n = len / SSSSRV_PACKET_MEM_SIZE + 1; totlen += n * SSSSRV_PACKET_MEM_SIZE; if (totlen < len) { return EINVAL;len = 1024 n = 1024 / 512 +1 n = 3 totlen = 3* 512 totlen = 1536
and sufficient length is 1024
what about
int n = (len + SSSSRV_PACKET_MEM_SIZE -1) / SSSSRV_PACKET_MEM_SIZE;
No we are not aiming at perfect match in case len is a perfect multiple of SSSSRV_PACKET_MEM_SIZE, 1536 is a good answer as it overallocates the buffer and gives us more space to work with.
Overallocating by a full chunk is ok and desirable as it allow the body to grow without yet another allocation. If we wanted perfect buffer lengths we wouldn't allocate in chunks.
Ack to Pavel's patch.
Simo.
Pushed to master, sssd-1-10 and sssd-1-9
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Simo Sorce