TS-4313: Fix improper type conversion

[maskit]

https://issues.apache.org/jira/browse/TS-4313

[zwoop]

Nice catch! +1. I think we should mark this as a possibly back port to 6.1.2 as well.

Do you know if this bug is in 5.3.x as well? If so, it likely should go there as well.

[jpeach]

This doesn't seem to be needed?

[jpeach]

ptrdiff_t is the right type for pointer arithmetic.

That said, I don't really understand the problem here. If field is not in the MIMEFieldBlockImpl you are scanning, then the pointer arithmetic is not valid in the first place. For the pointer arithmetic to overflow 32bits, the distance between the pointers would need to be 2^32 * sizeof(MIMEFieldBlockImpl), which seems unlikely.

A potential real problem here could be the compiler assuming that the comparison can't happen, since pointer arithmetic across different objects is not standards compliant, see here. To address this, I'd add MIMEFieldBlockImpl::contains(MIMEField*),

int
mime_hdr_field_slotnum(MIMEHdrImpl *mh, MIMEField *field)
{
  int slots_so_far;
  MIMEFieldBlockImpl *fblock;

  slots_so_far = 0;
  for (fblock = &(mh->m_first_fblock); fblock != NULL; fblock = fblock->m_next) {
    if (block->contains(field)) {
        MIMEField *first = &(fblock->m_field_slots[0]);
        ptrdiff_t block_slot = field - first; // in units of MIMEField
        return (slots_so_far + block_slot);
    }

    slots_so_far += MIME_FIELD_BLOCK_SLOTS;
  }
  return -1;
}

Can you think of a way to write tests for this?

[maskit]

@jpeach
I added some codes for debug and got these output. The rare case is happening.
(I built with --enable-debug and ASAN using clang on OSX.)

Output:

mime_hdr_set_accelerators_and_presence_bits(hdr 0x61d0001ab908, field 0x6250001aba98): &(mh->m_first_fblock.m_field_slots[0]) = 0x61d0001ab958
(int)(field - &(mh->m_first_fblock.m_field_slots[0])) = 0xA
(long)(field - &(mh->m_first_fblock.m_field_slots[0])) = 0x40000000A
(ptrdiff_t)(field - &(mh->m_first_fblock.m_field_slots[0])) = 0x40000000A
mime_hdr_field_slotnum(hdr 0x61d0001ab908, field 0x6250001ab998): first = 0x61d0001ab958
(int)(field - first) = 0x2
(long)(field - first) = 0x400000002
(ptrdiff_t)(field - first) = 0x400000002

Debug code1:

if ((int)(field - &(mh->m_first_fblock.m_field_slots[0])) >= 0 && (int)(field - &(mh->m_first_fblock.m_field_slots[0])) < MIME_FIELD_SLOTNUM_UNKNOWN && slot_num > UINT_MAX) {
  fprintf(stderr, "mime_hdr_set_accelerators_and_presence_bits(hdr %p, field %p): &(mh->m_first_fblock.m_field_slots[0]) = %p\n", mh, field, &(mh->m_first_fblock.m_field_slots[0]));
  fprintf(stderr, "(int)(field - &(mh->m_first_fblock.m_field_slots[0])) = 0x%X\n", (int)(field - &(mh->m_first_fblock.m_field_slots[0])));
  fprintf(stderr, "(long)(field - &(mh->m_first_fblock.m_field_slots[0])) = 0x%lX\n", (long)(field - &(mh->m_first_fblock.m_field_slots[0])));
  fprintf(stderr, "(ptrdiff_t)(field - &(mh->m_first_fblock.m_field_slots[0])) = 0x%tX\n", (ptrdiff_t)(field - &(mh->m_first_fblock.m_field_slots[0])));
}

Debug code2:

if ((int)(field - first) >= 0 && (int)(field - first) < MIME_FIELD_BLOCK_SLOTS && block_slot > UINT_MAX) {
  fprintf(stderr, "mime_hdr_field_slotnum(hdr %p, field %p): first = %p\n", mh, field, first);
  fprintf(stderr, "(int)(field - first) = 0x%X\n", (int)(field - first));
  fprintf(stderr, "(long)(field - first) = 0x%lX\n", (long)(field - first));
  fprintf(stderr, "(ptrdiff_t)(field - first) = 0x%tX\n", (ptrdiff_t)(field - first));
}

However the problem doesn't seems to be caused by the optimization, adding MIMEFieldBlockImpl::contains(MIMEField*) is a good idea. It's readable and reusable. I'll add it.

As for the tests, I had no idea. I came across the bug while I was testing HPACK, which just uses a MIMEHdr heavily. It may be possible to write tests for MIMEFieldBlockImpl::contains(MIMEField*). The test would be:

1. Create a mime field
2. Add 2^32 to the pointer of the mime field
3. Pass the modified pointer to contains()
4. Check if the result is false

@zwoop
The type conversions are in the initial commit on the git repo, so I think all versions should affect.

[jpeach]

Oh interesting. It looks like the pointers are ~500GB apart, so that certainly confirms the problem you found! I wonder if that is specific to the ASAN virtual address space?

Anyway, it sounds like we agree on adding MIMEFieldBlockImpl::contains(MIMEField*) ... sounds great :)

[maskit]

I've added contains() and tests for this.
@jpeach Can you review them?

[jpeach]

const MIMEField *field