1252795 - Collapse ssl3 union and struct for sslSessionIDStr and sidCacheEntryStr

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Tim Taubert [:ttaubert] (inactive)]

sslSessionIDStr in sslimpl.h and sidCacheEntryStr in sslsnce.c both have a:

union {
  struct {
  } ssl3;
} u;

Before bug 1228555 these were unions of structs for ssl2 and ssl3, now that only ssl3 is left we should collapse them and simplify the structure.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

The definition of sidCacheEntryStr is:

struct sidCacheEntryStr {
    /* 16 */ PRIPv6Addr addr; /* client's IP address */
    /*  4 */ PRUint32 creationTime;
    /*  4 */ PRUint32 lastAccessTime;
    /*  4 */ PRUint32 expirationTime;
    /*  2 */ PRUint16 version;
    /*  1 */ PRUint8 valid;
    /*  1 */ PRUint8 sessionIDLength;
    /* 32 */ PRUint8 sessionID[SSL3_SESSIONID_BYTES];
    /*  2 */ PRUint16 authAlgorithm;
    /*  2 */ PRUint16 authKeyBits;
    /*  2 */ PRUint16 keaType;
    /*  2 */ PRUint16 keaKeyBits;
    /* 72  - common header total */

    union {
      struct {
        /*  2 */ ssl3CipherSuite cipherSuite;
        /*  2 */ PRUint16 compression; /* SSLCompressionMethod */

        /* 54 */ ssl3SidKeys keys; /* keys, wrapped as needed. */

        /*  4 */ PRUint32 masterWrapMech;
        /*  4 */ SSL3KEAType exchKeyType;
        /*  4 */ PRInt32 certIndex;
        /*  4 */ PRInt32 srvNameIndex;
        /* 32 */ PRUint8 srvNameHash[SHA256_LENGTH]; /* SHA256 name hash */
      /*108 */} ssl3;

      /* force sizeof(sidCacheEntry) to be a multiple of cache line size */
      struct {
        /*120 */ PRUint8 filler[120]; /* 72+120==192, a multiple of 16 */
      } forceSize;
    } u;
};

If I put all the fields of sidCacheEntryStr.u.ssl3 in sidCacheEntryStr directly then it's quite hard to enforce sizeof(sidCacheEntryStr) % 16 == 0. For example ssl3SidKeys is 53 bytes but itself padded to 54 bytes. In sidCacheEntryStr however it's padded to 56 bytes, and afaik this behavior is quite compiler-dependent.

I don't have a good idea currently how we could merge these fields into sidCacheEntryStr and at the same time ensure that sizeof(sidCacheEntryStr) == 192 (or any other multiple of 16), a static padding size like the 120 before won't work I guess.

MT, any thoughts?

Comment 2

[Eric Rescorla (:ekr)]

Do we ever do pointer arithmetic on these? If not, we could just have two structs,
one you allocate (which is the right size) and one you operate on.

Comment 3

[Martin Thomson [:mt:]]

I as going to suggest something even simpler, which is to allocate based on ceil(sizeof(s))

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Ok, so the problem is that we don't actually allocate entries themselves, we have a big buffer and then do pointer arithmetic. Because that's baked into quite a few places both suggestions I think are complicated and I came up with something different:

https://nss-dev.phacility.com/D40

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=b52a60cf6a2b3f23a424710493beb938939c4b70

Comment 5

[Martin Thomson [:mt:]]

r+