Is this an incorrect Qualcomm usage or a glibc bug?

[honan li]

2017-09-01 17:00 GMT+08:00 Florian Weimer <fweimer@redhat.com>:
> On 09/01/2017 10:34 AM, honan li wrote:
>> HI,
>>
>> https://patchwork.sourceware.org/patch/12453/
>> This modification swapped __ss_align and __ss_padding in struct
>> sockaddr_storage. The Qualcomm platform I'm now developing on has lots
>> of typecast as follows.
>>
>> struct sockaddr_storage prefix_addr //IPV6 address is stored in
>> prefix_addr.__ss_padding
>> (struct sockaddr_in6 *)&(prefix_addr)->sin6_addr.s6_addr
>
> Please quote actual source doe.  The above snippet seems to have been
> garbled.
>
>> Is Qualcomm's usage of typecast wrong, or the usage is reasonable but
>> glibc missed to consider this scenario?
>
> Access to __ struct members is generally invalid.  Based on the
> information you posted, I still think the glibc change was technically
> valid.
>
> Maybe we should rename the fields to reflect their changed offsets, so
> that applications which access these __ members run into compiler errors
> when being recompiled, instead of silent miscompilation.
>
> Thanks,
> Florian

Hi, Florian,

It's a great idea to remind others the change of offset.

> Please quote actual source doe.  The above snippet seems to have been
> garbled.

Here is source code with the fault mentioned extracted from our
Qualcomm platform, hopefully it is clear enough.

#define SASTORAGE_DATA(addr)    (addr).__ss_padding

typedef struct qcmap_cm_nl_prefix_info_s {
  boolean prefix_info_valid;
  unsigned char prefix_len;
  unsigned int mtu;
  struct sockaddr_storage prefix_addr;
  struct ifa_cacheinfo          cache_info;
} qcmap_cm_nl_prefix_info_t;

void QCMAP_Backhaul::GetIPV6PrefixInfo(char *devname,
                                       qcmap_cm_nl_prefix_info_t
*ipv6_prefix_info)
{
  struct sockaddr_in6 *sin6 = NULL;

  ...

  sin6 = (struct sockaddr_in6 *)&ipv6_prefix_info->prefix_addr;
  memcpy(SASTORAGE_DATA(ipv6_prefix_info->prefix_addr),
         RTA_DATA(rta),
         sizeof(sin6->sin6_addr));
  ...
}

int QCMAP_Backhaul::UpdatePrefix
(
  qcmap_cm_nl_prefix_info_t *ipv6_prefix_info,
  boolean deprecate, boolean send_ra,
  uint8_t *dest_v6_ip
)
{

  ...

  memcpy(prefix_info->nd_opt_pi_prefix.s6_addr,
         ((struct sockaddr_in6
*)&(ipv6_prefix_info->prefix_addr))->sin6_addr.s6_addr,
         sizeof(prefix_info->nd_opt_pi_prefix.s6_addr));
  ...
}

boolean QCMAP_Backhaul::EnableIPV6Forwarding()
{
  ...
  QcMapBackhaulMgr->GetIPV6PrefixInfo(devname,
&QcMapBackhaulMgr->ipv6_prefix_info);
  ...
  QcMapBackhaulMgr->UpdatePrefix(&QcMapBackhaulMgr->ipv6_prefix_info,
false, true, NULL);
  ...
}



The error occurs in UpdatePrefix.

> Access to __ struct members is generally invalid.

Do you mean SASTORAGE_DATA above should not directly access
__ss_padding? Could your please recommend a good usage or kindly give
a reference link?

Thanks,
Honan