unit3214: avoid a false failure on CHERI

[collinfunk]

On CHERI pointers are 128 bits [1]. This causes the unit3214 test to fail, which was written with more traditional platforms in mind. Here is the output of log/stderr3214:

URL: -
BAD: struct Curl_easy is 7984 bytes, allowed to be 5370: 2614 bytes too big
BAD: struct connectdata is 1408 bytes, allowed to be 1300: 108 bytes too big
BAD: struct Curl_multi is 1248 bytes, allowed to be 850: 398 bytes too big
BAD: struct curl_httppost is 224 bytes, allowed to be 112: 112 bytes too big
BAD: struct curl_slist is 32 bytes, allowed to be 16: 16 bytes too big
BAD: struct curl_khkey is 32 bytes, allowed to be 24: 8 bytes too big
BAD: struct curl_hstsentry is 48 bytes, allowed to be 40: 8 bytes too big
BAD: struct curl_mime is 144 bytes, allowed to be 96: 48 bytes too big
BAD: struct curl_mimepart is 592 bytes, allowed to be 440: 152 bytes too big
BAD: struct curl_certinfo is 32 bytes, allowed to be 16: 16 bytes too big
BAD: struct curl_tlssessioninfo is 32 bytes, allowed to be 16: 16 bytes too big
BAD: struct curl_blob is 32 bytes, allowed to be 24: 8 bytes too big
BAD: struct CURLMsg is 48 bytes, allowed to be 24: 24 bytes too big
BAD: struct curl_header is 80 bytes, allowed to be 48: 32 bytes too big
Test ended with result 14

Since most platforms will have 32-bit or 64-bit bits, doubling the allowed size on CHERI seems reasonable and allows the test to pass.

[1] https://www.cl.cam.ac.uk/research/security/ctsrd/pdfs/20171017a-cheri-poster.pdf

[vszakats]

Is there perhaps a generic way to detect all systems with 128-bit pointers?

[collinfunk]

I considered doing this:

if (sizeof(void *) > 8)
  allowed *= sizeof(void *) / 8;

But I figured we might want to be more careful and let other strange systems fail until someone tests them. FWIW, CHERI had no issues other than this when I ran the test suite.

[bagder]

Is there perhaps a generic way to detect all systems with 128-bit pointers?

With configure it is as easy as adding:

CURL_SIZEOF(time_t)

When then creates a define in curl_config.h:

/* Size of char * in number of bytes */
#define SIZEOF_CHAR_P 8

I doubt there is a reliable way that does not use the compiler to determine this.

[collinfunk]

My personal preference is the way it is done in my pull request. The configure test works, but they you would need extra preprocessor conditionals along with the configure.ac/CMakeLists.txt changes.

E.g., to avoid unused function warnings you would need:

#if SIZEOF_CHAR_P <= 8
static void checksize(const char *name, size_t size, size_t allowed)
{
  [...]
}
#endif

static CURLcode test_unit3214(const char *arg)
{
#if SIZEOF_CHAR_P <= 8
  /* Perform the tests. */
  [...]
#endif
}

Let me know if you disagree and I can adjust it to follow your suggestion.

[bagder]

I consider the main point of this PR to be to avoid tripping the test, so I'm fine with just doubling the allowed size on 128-bit pointer architectures, since they are still a rare exception. We can consider something else in a future if that changes.

[dfandrich]

Since this isn't Cheri-specific but would affect other systems with large pointers (IBM i? Elbrus? RV128I?), how about guarding this with a more generic condition, like #if SIZEOF_CURL_OFF_T > 8 or (since I suspect SIZEOF_CURL_OFF_T won't actually work to detect this) creating a configure test that directly tests for pointer size?

[dfandrich]

Given that this is merely test code, that run-time check is fine to me. But add a comment mentioning CHERI and the root cause of the problem. There's no need to wait for other such architectures to fail first because we know the problem will affect them. Thinking about it some more, maybe it's worthwhile reducing the size in the case of systems with 32 bit pointers. And multiplying by 2 for 128-bit pointer systems is likely overkill because much of the struct size consists of integers, not pointers, and those are still 64 bits on 128-bit-pointer systems. Actually, this is supposed to be just a gross size sanity check so I'm going to reign in my thoughts and not try to microoptimize this. But please add a comment above the size settings in that test mentioning that the sizes are intended for systems with 64 bit integers and 64 bit pointers.

[collinfunk]

Given that this is merely test code, that run-time check is fine to me. But add a comment mentioning CHERI and the root cause of the problem. There's no need to wait for other such architectures to fail first because we know the problem will affect them.

Makes sense.

Thinking about it some more, maybe it's worthwhile reducing the size in the case of systems with 32 bit pointers. And multiplying by 2 for 128-bit pointer systems is likely overkill because much of the struct size consists of integers, not pointers, and those are still 64 bits on 128-bit-pointer systems. Actually, this is supposed to be just a gross size sanity check so I'm going to reign in my thoughts and not try to microoptimize this.

Right, you would have to keep track of how many pointers are in each struct. It didn't seem like this test was intended to be super strict like that. It seemed like the intention was to have a rough sanity check to avoid large changes in size.

But please add a comment above the size settings in that test mentioning that the sizes are intended for systems with 64 bit integers and 64 bit pointers.

I only mentioned 64 bit pointers. I felt like "64 bit integers" might be interpreted as saying int is 64 bits. Happy to reword it if you'd like.

[bagder]

Another way would be to just disable the test on such systems, as the problem will be detected on 64-bit pointer platforms anyway...

[dfandrich]

This looks fine to me now.

[vszakats]

Thanks @collinfunk, merged now.