Hi Roel, Looking at slpd_knownda.c:564, I have to say I'm a little confused as to why this line of code even exists. It appears to be a duplicate of the line above it using an alias. As I started to say in my previous message, sockaddr was originally a generic type large enough to hold all known address types at the time it was invented. Today, with many more address types available, a new generic buffer type is needed - that's why they invented sockaddr_storage - I believe it will hold addresses up to 128 bytes long.
All of the sockaddr family of structures share a common prefix - a 16-bit value called "family". In fact, in the latest gcc header files, this common prefix is defined within each address structure using a single macro that names the common "family" field according to the address type - something like "unsigned short <type>_family". For sockaddr_storage, the prefix ss is used for <type>, so the field becomes ss_family, but in ipv4 addresses using sockaddr_in, the prefix is sin, so the field is named sin_family. In other words, line 563 sets the address family in the sockaddr_storage address buffer to AF_INET6, then in the very next line, the same value is written over the same location - or rather, that is apparently the author's intent, but given strict aliasing, the compiler is allowed to assume that the location referred to by lines 563 and 564 are not the same because they're referred to with pointers of different types. In fact, this is harmless because the strict-aliasing rules only affect things like variable hoisting (where variables assigned within a loop may be hoisted out of the loop for performance's sake if the compiler can detect that the assignment is redundant - with strict aliasing rules, the compiler is allowed to make much broader assumptions). I think we should rework this code a bit - remove the extra type-casted assignments - they really are totally redundant, as far as I can tell. John -----Original Message----- From: Roel van de Kraats [mailto:rkra...@dds.nl] Sent: Wednesday, May 25, 2011 1:03 PM To: John Calcote Cc: openslp-devel@lists.sourceforge.net Subject: Re: [Openslp-devel] Bug in libslp_knownda.c Thanks for clarifying this, John. Although everything seems to work fine, I'm a bit worried about the 'strict-aliasing' warnings my gcc compiler gives, like this one: slpd_knownda.c:564: warning: dereferencing pointer 'daaddr.53' does break strict-aliasing rules With the used optimization levels, it is possible that modifying an 'alias' is optimized away completely, since the compiler assumes its value isn't used anymore. See e.g. http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-al iasing.html We could use the '-fno-strict-aliasing' compiler option, but I'd prefer to use a 'cleaner' way. I'm not sure if a 'double cast' through void* is good enough. BR, Roel On 05/25/2011 05:25 PM, John Calcote wrote: > This has been a minor sore point for decades in the socket interface - > how to refer to a structure that could hold any of a number of > different address types. Early on, it was decided just to refer to a > sockaddr object - an object of a well-known type and size - too small > these days, but at least useful for ipv4 addresses. This was done > because void* was not actually added to the C standard till 1989 when > the C89 standard came out. Prior to that, your only option was to typecast. > > In short, this is exactly the sort of thing for which void * was > originally introduced. You pass a generic pointer to the data > structure, along with a type discriminator and a buffer length. In > this case, the type discriminator is always at a well-known offset > within the buffer referred to, and the size is a separate argument. > Had void* been available when the sockets interface was being > designed, I believe they would have used it instead of passing > sockaddr pointers around, because in the C language, you can > automatically cast to and from void (not so in C++, of course, but not > relevant here), so you can simply use a void* as if it were any of the sockaddr family of structures. > > Is it type safe? Of course not. You can just as easily auto-cast it to > the wrong type of thing. But given the fact that the original socket > spec has you manually type-casting from one type of structure to > another all over the place, it's at least as safe, but much more convenient. > > John > > -----Original Message----- > From: Roel van de Kraats [mailto:rkra...@dds.nl] > Sent: Wednesday, May 25, 2011 6:06 AM > To: 'openslp-devel@lists.sourceforge.net' > Subject: Re: [Openslp-devel] Bug in libslp_knownda.c > > > > On 05/25/2011 01:18 PM, Morrell Richard (external) wrote: >> Hi Roel, >> >> The change in r1597 was only supposed to abstract the cache refresh >> code into a separate function, not change anything else, so the >> change to the address size in the second call was incorrect. Please >> go ahead with the update. > Done. See r1673. > > In my opinion it is a bit 'dangerous' to pass through void pointers > like this. Wouldn't it be better to explicitly use 'struct sockaddr_storage' > pointers? Or even use a union throughout the whole source tree to hold > the different 'sockaddr' types, like > http://svn.tartarus.org/sgt/putty/unix/uxnet.c?p2=%2Fputty%2Funix%2Fux > net.c& > p1=putty%2Funix%2Fuxnet.c&r1=8612&r2=8611&view=diff&pathrev=8612 > ? > > BR, > Roel >> Regards, >> >> Richard >> >> -----Original Message----- >> From: Roel van de Kraats [mailto:rkra...@dds.nl] >> Sent: 25 May 2011 11:34 >> To: 'openslp-devel@lists.sourceforge.net' >> Subject: [Openslp-devel] Bug in libslp_knownda.c >> >> >> Dear all, >> >> It seems that libslp_knownda.c has a bug on line 716 (see >> > http://openslp.svn.sourceforge.net/viewvc/openslp/trunk/openslp/libslp > /libsl >> p_knownda.c?view=markup&pathrev=1672), >> because sizeof(struct in_addr) is too small to copy the whole address. >> This causes 'slptool findsrvtypes' to fail in case another host acts >> as DA. This was introduced in r1597 (see >> > http://openslp.svn.sourceforge.net/viewvc/openslp?view=revision&revisi > on=159 >> 7). >> >> I guess it is safe to replace 'sizeof (struct in_addr)' with >> 'sizeof(struct sockaddr_storage)', but I'm not sure. If anyone can >> confirm this, I'll commit the fix. >> >> BR, >> Roel >> >> >> >> > ---------------------------------------------------------------------- > ------ >> -- >> vRanger cuts backup time in half-while increasing security. >> With the market-leading solution for virtual backup and recovery, you >> get blazing-fast, flexible, and affordable data protection. >> Download your free trial now. >> http://p.sf.net/sfu/quest-d2dcopy1 >> _______________________________________________ >> Openslp-devel mailing list >> Openslp-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/openslp-devel >> >> This email, including any attachment, is a confidential communication >> intended solely for the use of the individual or entity to whom it is >> addressed. It contains information which is private and may be >> proprietary or covered by legal professional privilege. If you have >> received this > email >> in error, please notify the sender upon receipt, and immediately >> delete it from your system. >> >> Anything contained in this email that is not connected with the >> businesses of this company is neither endorsed by nor is the >> liability of this > company. >> Whilst we have taken reasonable precautions to ensure that any >> attachment > to >> this email has been swept for viruses, we cannot accept liability for >> any damage sustained as a result of software viruses, and would >> advise that > you >> carry out your own virus checks before opening any attachment. >> >> >> > ---------------------------------------------------------------------- > ------ > -- >> vRanger cuts backup time in half-while increasing security. >> With the market-leading solution for virtual backup and recovery, you >> get blazing-fast, flexible, and affordable data protection. >> Download your free trial now. >> http://p.sf.net/sfu/quest-d2dcopy1 >> _______________________________________________ >> Openslp-devel mailing list >> Openslp-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/openslp-devel > > ---------------------------------------------------------------------- > ------ > -- > vRanger cuts backup time in half-while increasing security. > With the market-leading solution for virtual backup and recovery, you > get blazing-fast, flexible, and affordable data protection. > Download your free trial now. > http://p.sf.net/sfu/quest-d2dcopy1 > _______________________________________________ > Openslp-devel mailing list > Openslp-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openslp-devel > ------------------------------------------------------------------------------ vRanger cuts backup time in half-while increasing security. With the market-leading solution for virtual backup and recovery, you get blazing-fast, flexible, and affordable data protection. Download your free trial now. http://p.sf.net/sfu/quest-d2dcopy1 _______________________________________________ Openslp-devel mailing list Openslp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openslp-devel
