5:46 a.m.
The sss_client was copying 32bit port value, but the NSS responder was
reading 16bit port value. This was breaking on Big-Endian machines where
we read "the other 16bits".
By the way, is there a reason to use 32bits in the client in the first
place? IIRC a port number is a 16 bit value..
6:28 a.m.
On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
The sss_client was copying 32bit port value, but the NSS responder
was
reading 16bit port value. This was breaking on Big-Endian machines where
we read "the other 16bits".
By the way, is there a reason to use 32bits in the client in the first
place? IIRC a port number is a 16 bit value..
No, you're right. The client should only be sending a 16-bit value.
Nack.
Please change the client to send a uint16_t instead.
6:56 a.m.
On Fri, May 25, 2012 at 07:28:06AM -0400, Stephen Gallagher wrote:
On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
> The sss_client was copying 32bit port value, but the NSS responder was
> reading 16bit port value. This was breaking on Big-Endian machines where
> we read "the other 16bits".
>
> By the way, is there a reason to use 32bits in the client in the first
> place? IIRC a port number is a 16 bit value..
No, you're right. The client should only be sending a 16-bit value.
Nack.
Please change the client to send a uint16_t instead.
Attached.
7:09 a.m.
On Fri, 2012-05-25 at 13:56 +0200, Jakub Hrozek wrote:
On Fri, May 25, 2012 at 07:28:06AM -0400, Stephen Gallagher wrote:
> On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
> > The sss_client was copying 32bit port value, but the NSS responder was
> > reading 16bit port value. This was breaking on Big-Endian machines where
> > we read "the other 16bits".
> >
> > By the way, is there a reason to use 32bits in the client in the first
> > place? IIRC a port number is a 16 bit value..
>
> No, you're right. The client should only be sending a 16-bit value.
>
> Nack.
> Please change the client to send a uint16_t instead.
Attached.
Nack (minor).
Would you mind using SAFEALIGN_SET_UINT16() for the padding? The macro
expands to exactly the same code you have there.
Has this been tested on a little-endian and big-endian system?
7:12 a.m.
On Fri, 2012-05-25 at 08:09 -0400, Stephen Gallagher wrote:
On Fri, 2012-05-25 at 13:56 +0200, Jakub Hrozek wrote:
> On Fri, May 25, 2012 at 07:28:06AM -0400, Stephen Gallagher wrote:
> > On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
> > > The sss_client was copying 32bit port value, but the NSS responder was
> > > reading 16bit port value. This was breaking on Big-Endian machines where
> > > we read "the other 16bits".
> > >
> > > By the way, is there a reason to use 32bits in the client in the first
> > > place? IIRC a port number is a 16 bit value..
> >
> > No, you're right. The client should only be sending a 16-bit value.
> >
> > Nack.
> > Please change the client to send a uint16_t instead.
>
> Attached.
Nack (minor).
Would you mind using SAFEALIGN_SET_UINT16() for the padding? The macro
expands to exactly the same code you have there.
Has this been tested on a little-endian and big-endian system?
Replying to myself, I can confirm that this is working on x86_64 at
least.
7:22 a.m.
On Fri, May 25, 2012 at 08:12:56AM -0400, Stephen Gallagher wrote:
On Fri, 2012-05-25 at 08:09 -0400, Stephen Gallagher wrote:
> On Fri, 2012-05-25 at 13:56 +0200, Jakub Hrozek wrote:
> > On Fri, May 25, 2012 at 07:28:06AM -0400, Stephen Gallagher wrote:
> > > On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
> > > > The sss_client was copying 32bit port value, but the NSS responder
was
> > > > reading 16bit port value. This was breaking on Big-Endian machines
where
> > > > we read "the other 16bits".
> > > >
> > > > By the way, is there a reason to use 32bits in the client in the
first
> > > > place? IIRC a port number is a 16 bit value..
> > >
> > > No, you're right. The client should only be sending a 16-bit value.
> > >
> > > Nack.
> > > Please change the client to send a uint16_t instead.
> >
> > Attached.
>
>
> Nack (minor).
>
> Would you mind using SAFEALIGN_SET_UINT16() for the padding? The macro
> expands to exactly the same code you have there.
>
Sure, new patch is attached.
> Has this been tested on a little-endian and big-endian system?
Replying to myself, I can confirm that this is working on x86_64 at
least.
Yes, I also tested on s390x running RHEL6.
7:45 a.m.
On Fri, 2012-05-25 at 14:22 +0200, Jakub Hrozek wrote:
On Fri, May 25, 2012 at 08:12:56AM -0400, Stephen Gallagher wrote:
> On Fri, 2012-05-25 at 08:09 -0400, Stephen Gallagher wrote:
> > On Fri, 2012-05-25 at 13:56 +0200, Jakub Hrozek wrote:
> > > On Fri, May 25, 2012 at 07:28:06AM -0400, Stephen Gallagher wrote:
> > > > On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
> > > > > The sss_client was copying 32bit port value, but the NSS
responder was
> > > > > reading 16bit port value. This was breaking on Big-Endian
machines where
> > > > > we read "the other 16bits".
> > > > >
> > > > > By the way, is there a reason to use 32bits in the client in the
first
> > > > > place? IIRC a port number is a 16 bit value..
> > > >
> > > > No, you're right. The client should only be sending a 16-bit
value.
> > > >
> > > > Nack.
> > > > Please change the client to send a uint16_t instead.
> > >
> > > Attached.
> >
> >
> > Nack (minor).
> >
> > Would you mind using SAFEALIGN_SET_UINT16() for the padding? The macro
> > expands to exactly the same code you have there.
> >
Sure, new patch is attached.
> > Has this been tested on a little-endian and big-endian system?
>
> Replying to myself, I can confirm that this is working on x86_64 at
> least.
Yes, I also tested on s390x running RHEL6.
Ack
7:52 a.m.
On Fri, 2012-05-25 at 08:45 -0400, Stephen Gallagher wrote:
On Fri, 2012-05-25 at 14:22 +0200, Jakub Hrozek wrote:
> On Fri, May 25, 2012 at 08:12:56AM -0400, Stephen Gallagher wrote:
> > On Fri, 2012-05-25 at 08:09 -0400, Stephen Gallagher wrote:
> > > On Fri, 2012-05-25 at 13:56 +0200, Jakub Hrozek wrote:
> > > > On Fri, May 25, 2012 at 07:28:06AM -0400, Stephen Gallagher wrote:
> > > > > On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
> > > > > > The sss_client was copying 32bit port value, but the NSS
responder was
> > > > > > reading 16bit port value. This was breaking on Big-Endian
machines where
> > > > > > we read "the other 16bits".
> > > > > >
> > > > > > By the way, is there a reason to use 32bits in the client
in the first
> > > > > > place? IIRC a port number is a 16 bit value..
> > > > >
> > > > > No, you're right. The client should only be sending a 16-bit
value.
> > > > >
> > > > > Nack.
> > > > > Please change the client to send a uint16_t instead.
> > > >
> > > > Attached.
> > >
> > >
> > > Nack (minor).
> > >
> > > Would you mind using SAFEALIGN_SET_UINT16() for the padding? The macro
> > > expands to exactly the same code you have there.
> > >
>
> Sure, new patch is attached.
>
> > > Has this been tested on a little-endian and big-endian system?
> >
> > Replying to myself, I can confirm that this is working on x86_64 at
> > least.
>
> Yes, I also tested on s390x running RHEL6.
Ack
Pushed to master and sssd-1-8.
9:17 a.m.
On Fri, 2012-05-25 at 08:52 -0400, Stephen Gallagher wrote:
On Fri, 2012-05-25 at 08:45 -0400, Stephen Gallagher wrote:
> On Fri, 2012-05-25 at 14:22 +0200, Jakub Hrozek wrote:
> > On Fri, May 25, 2012 at 08:12:56AM -0400, Stephen Gallagher wrote:
> > > On Fri, 2012-05-25 at 08:09 -0400, Stephen Gallagher wrote:
> > > > On Fri, 2012-05-25 at 13:56 +0200, Jakub Hrozek wrote:
> > > > > On Fri, May 25, 2012 at 07:28:06AM -0400, Stephen Gallagher
wrote:
> > > > > > On Fri, 2012-05-25 at 12:46 +0200, Jakub Hrozek wrote:
> > > > > > > The sss_client was copying 32bit port value, but the
NSS responder was
> > > > > > > reading 16bit port value. This was breaking on
Big-Endian machines where
> > > > > > > we read "the other 16bits".
> > > > > > >
> > > > > > > By the way, is there a reason to use 32bits in the
client in the first
> > > > > > > place? IIRC a port number is a 16 bit value..
> > > > > >
> > > > > > No, you're right. The client should only be sending a
16-bit value.
> > > > > >
> > > > > > Nack.
> > > > > > Please change the client to send a uint16_t instead.
> > > > >
> > > > > Attached.
> > > >
> > > >
> > > > Nack (minor).
> > > >
> > > > Would you mind using SAFEALIGN_SET_UINT16() for the padding? The
macro
> > > > expands to exactly the same code you have there.
> > > >
> >
> > Sure, new patch is attached.
> >
> > > > Has this been tested on a little-endian and big-endian system?
> > >
> > > Replying to myself, I can confirm that this is working on x86_64 at
> > > least.
> >
> > Yes, I also tested on s390x running RHEL6.
>
> Ack
Pushed to master and sssd-1-8.
Simo noted that we changed the protocol here. We dropped 32 bits of
padding. In order to remain compatible with existing clients, we need to
put that back in, which the attached patch does. (It also corrects the
comment describing the protocol to have it match reality).
The reason we want to do this is that an upgrade and restart of SSSD
won't change the copy of libnss_sss.so that's already loaded into
running processes. Thus we would be breaking service lookups until those
apps were restarted.
10:54 a.m.
On Fri, 2012-05-25 at 10:17 -0400, Stephen Gallagher wrote:
Simo noted that we changed the protocol here. We dropped 32 bits of
padding. In order to remain compatible with existing clients, we need
to
put that back in, which the attached patch does. (It also corrects the
comment describing the protocol to have it match reality).
The reason we want to do this is that an upgrade and restart of SSSD
won't change the copy of libnss_sss.so that's already loaded into
running processes. Thus we would be breaking service lookups until
those
apps were restarted.
ACK.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11:36 a.m.
On Fri, May 25, 2012 at 11:54:40AM -0400, Simo Sorce wrote:
On Fri, 2012-05-25 at 10:17 -0400, Stephen Gallagher wrote:
> Simo noted that we changed the protocol here. We dropped 32 bits of
> padding. In order to remain compatible with existing clients, we need
> to
> put that back in, which the attached patch does. (It also corrects the
> comment describing the protocol to have it match reality).
>
> The reason we want to do this is that an upgrade and restart of SSSD
> won't change the copy of libnss_sss.so that's already loaded into
> running processes. Thus we would be breaking service lookups until
> those
> apps were restarted.
>
ACK.
Simo.
Ack++
I tested this by writing a program that called getservbyport(port,proto) in
a loop and went from unpatched SSSD to my patch and then to Stephen's patch.
After my patch was applied and SSSD restarted, the sss_nss client was
receiving "NULL" instead of "proto" in the protocol field - it was
probably hitting the padding. Then I applied Stephen's patch, restarted
SSSD and everything went back to normal.
Thanks for the explanation Simo.
12:31 p.m.
On Fri, 2012-05-25 at 18:36 +0200, Jakub Hrozek wrote:
On Fri, May 25, 2012 at 11:54:40AM -0400, Simo Sorce wrote:
> On Fri, 2012-05-25 at 10:17 -0400, Stephen Gallagher wrote:
> > Simo noted that we changed the protocol here. We dropped 32 bits of
> > padding. In order to remain compatible with existing clients, we need
> > to
> > put that back in, which the attached patch does. (It also corrects the
> > comment describing the protocol to have it match reality).
> >
> > The reason we want to do this is that an upgrade and restart of SSSD
> > won't change the copy of libnss_sss.so that's already loaded into
> > running processes. Thus we would be breaking service lookups until
> > those
> > apps were restarted.
> >
> ACK.
>
> Simo.
>
Ack++
I tested this by writing a program that called getservbyport(port,proto) in
a loop and went from unpatched SSSD to my patch and then to Stephen's patch.
After my patch was applied and SSSD restarted, the sss_nss client was
receiving "NULL" instead of "proto" in the protocol field - it was
probably hitting the padding. Then I applied Stephen's patch, restarted
SSSD and everything went back to normal.
Thanks for the explanation Simo.
Pushed to master and sssd-1-8.
3900
days inactive
3900
days old
sssd-devel@lists.fedorahosted.org
11 comments
3 participants
participants (3)
-
Jakub Hrozek -
Simo Sorce -
Stephen Gallagher