Set a smaller default traversal limit for targets with short pointers

[saulrh]

The default traversal limit is set to 8 * 1024 * 1024, which is substantially larger than the entire address space on a machine with 16-bit pointers. rustc therefore refuses to compile capnpproto-rust for such targets.

This patch adds a conditional compilation directive that sets the default traversal limit to 1024 when the target_pointer_width is 16. I chose this value by looking at the size of the default limit relative to the size of the address space for each pointer width; if I've done my math right, the normal default takes up 26 of the 32 bits of address space available to a 32-bit computer (2^3 bytes/word * 2^3 * 2^10 * 2^10 words), so I chose 1024 as the constant to take up 13 of the 16 bits in a system with 16-bit pointers (2^3 bytes/word * 2^10 words).

I will say that the default value should rarely matter for this sort of target in the first place. For example, my project is running capnproto out of a 128-byte single segment allocator, which should run out well before the traversal limit does, and that's if any malicious protos somehow manage to sneak onto the i2c bus in the first place.

(yes, this is a fairly absurd endeavor. i don't care, i'm having fun. :V )

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.37%. Comparing base (ab342b3) to head (7d3e998).
Report is 125 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
+ Coverage   51.64%   52.37%   +0.73%     
==========================================
  Files          69       70       +1     
  Lines       33735    34587     +852     
==========================================
+ Hits        17422    18116     +694     
- Misses      16313    16471     +158     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dwrensha]

Hm... I wonder if it would be better to change traveral_limit_in_words from an Option<usize> to an Option<u64>. I think that would fix the compilation issue. I suppose one problem is that sync::ReadLimiter requires AtomicUsize:

capnproto-rust/capnp/src/private/read_limiter.rs

Line 31 in ffcb213

limit: AtomicUsize,

Probably whatever 16-bit platform you're using does not have AtomicU64.

[saulrh]

Probably whatever 16-bit platform you're using does not have AtomicU64.

The ATmega32U4 I'm targeting tops out at AtomicU8, yes. Gotta love arduino-compatible hardware.

[dwrensha]

The ATmega32U4 I'm targeting tops out at AtomicU8, yes.

Ah, so it probably does not have AtomicUsize, and therefore the sync_reader feature cannot be enabled anyway.

the normal default takes up 26 of the 32 bits of address space available to a 32-bit computer

I don't think this comparison is particularly meaningful.

I think the more relevant question is: how much do we want to allow a malicious message to spuriously consume cpu cycles?

I wonder whether it would make more sense to make the default limit simply u16::MAX as usize on 16-bit architectures.

[saulrh]

I think the more relevant question is: how much do we want to allow a malicious message to spuriously consume cpu cycles?

I wonder whether it would make more sense to make the default limit simply u16::MAX as usize on 16-bit architectures.

That would work too, yeah. Embedded targets are going to vary heavily in how they're invoking this kind of code; I don't think that may will be doing it inside interrupt handlers, for example, but some are going to be running it in hot loops and others will be using it infrequently for fancy logging. I could almost see configuring the system to require weird targets to set their own defaults somehow, but that might be super finicky or hard to document. Setting the default to u16::MAX might signal that well enough? "Here be monsters, if you want safety you'll have to do it yourself."

[dwrensha]

How about a limit of 8192 words? That many words would exactly fill the memory space, so there's no way a legitimate message would hit the limit.

Note that it's straightforward for downstream users to specify their own preferred values for ReaderOptions at point-of-use.

[saulrh]

That sounds entirely reasonable. Do you want me to squash into a single commit before you hit the merge button, do you have autosquash configured, or do you not squash at all?

[dwrensha]

Thanks!