Change return type of uint16 to uint32 in sn_nsdl_calculate_registrat…

[anttiylitokola]

…ion_body_size

• Fix error: IOTCLT-776 - mbed-client crash when registering with very big amount of instances

[anttiylitokola]

@yogpan01, @TeroJaasko

[kjbracey]

Why not size_t? uint32_t will be unnecessarily large and bloaty on 16-bit platforms.

In a lot of cases 16-bit sizes are used deliberately because there are some basically 16-bit platforms with some theoretical abillity to have >64K objects, that declare 32-bit size-t, but we know we won't be >64K and we want to avoid the code bloat of processing 32-bit values on those platforms.

If you ever find the 64K assumption broken, change to size_t, NOT uint32_t. There is a chance that size_t will be 16 bit on platforms with small RAM, so it stands a chance of being more efficient.

(Conversely it may be less efficient on 64-bit Linux builds, but that's not an issue. And it means that >4G stuff would work too...)

[kjbracey]

Actually though, looking at what the point of the change was, it is that you are deliberately trying to spot overflow of the return value. If size_t WAS 16-bit, the wrap test doesn't actually work.

One option is to keep using uint32_t internally, and range check against SIZE_MAX. Doesn't save the 32-bit arithmetic code bloat though.

The other option is to use size_t, but check for wrap while accumulating:

size_t total_size = 0;
while (blah) {
    calculate this_size;
    if (total_size + this_size < total_size) {
          // we overflowed size_t, indicate error
    }
    total_size += this_size;

}

[SeppoTakalo]

For study, follow there CERT-C rules:

• INT32-C Ensure that unsigned integer operations do not wrap
• INT01-C. Use rsize_t or size_t for all integer values representing the size of an object

[TeroJaasko]

I kind of liked the original commit, where the overflow check would be needed to be done only at one place. Using 16bit counter internally and checking for overflow after the at least four additions might not even be more optimal.

[kjbracey]

If it was a 16-bit platform with 32-bit size_t, holding 32-bit size would be quite suboptimal on that, as you'd doing the 32-bit arithmetic and holding 2 registers for the total.

The suboptimality (is that a word) on other platforms with register-sized size_t is miniscule - basically one comparison, and no extra register pressure.

And the overflow check is still technically valid! You /could/ overflow a 32-bit size_t, conceivably. Okay, it's a bit unlikely to bother with the check in most cases, but as you're putting it in for 16-bit, why not keep the logic active on all platforms?

[TeroJaasko]

True, it is possible to overflow the 32bit variable there too, but that check would be needed only once per loop?

[kjbracey]

Aren't all the checks once per loop?

Ah, I see, you're doing lots of bitty increments. Well, either a 32-bit or 16-bit total could overflow at any of those additions.

Assuming you could assume the size of 1 item didn't overflow, I would be accumulating this item's size into a variable in the loop, without any overflow checks on that, and then just the 1 overflow check as you add this item's size to the total accumulator in one go.

[kjbracey]

I guess super-paranoia about overflow is an argument for mixing types. If all your APIs used 16-bit sizes, and you internally always accumulated using uint32_t, before narrowing the result, that largely eliminates overflow issues.

(Compare libservice IP frame checksum code - it accumulates 16-bit words into a 32-bit accumulator, knowing it can't overflow a 32-bit accumulator with 64K of data).

But that trickery does means you're relying on a "smaller than the system supports" size limit built into your APIs. If you used size_t to properly support any size of data, you don't necessarily have a type bigger than size_t to accumulate into. :(

[TeroJaasko]

Yes, 32bit variable will overflow just as the smaller values too. But the overflow with 32bit could be checked only once per loop, as it could not overflow more than once per loop and get undetected. As far as I can see, the loop is adding 16bit values together. Edit: this is duplicate of what you commented on simultaneusly:)

[kjbracey]

As it stands, I don't see how you could check 32-bit overflow once per loop. It currently has no record of "size before this iteration" to check for overflow against - it's accumulating straight into the total.

I guess you're thinking:

 uint32_t total;
 while () {
       uint32_t old_total = total;
       total += x
       total += y;
       if (total < old_total) {
            overflow
       }
  }

But that's not much different from my proposal of:

 size_t total;
 while () {
       size_t this_item = 0;
       this_item += x
       this_item += y;
       if (total + this_item < total) {
            overflow
       }
      total += this_item;
  }

The difference being the former assumes 1 item fits in uint32_t, the latter assumes 1 item fits in size_t.

If you think 1 item fitting into size_t isn't a reasonable assumption, then I guess you could go with the former, but I'm feeling bad about locking 16-bit and 32-bit sizes in. The first version is inefficient on 16-bit processors with 16-bit size_t, and limits you to small items on 64-bit platforms, the second is portable.

[anttiylitokola]

rebuild