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..
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.
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.
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?
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.
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.
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
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.
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.
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.
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.
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.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Stephen Gallagher