3 ms·

You only need to bounds check once before a for loop starts, not every iteration.

card_zero 1y ago
If they're all being inserted contiguously. Anyway that's a form of saying "I know by reasoning that none of these will be outside the bounds, so let's not check".
ryao 1y ago
It depends on the loop. Here are a bunch of loops that need bounds checks on every loop iteration: https://github.com/openzfs/zfs/commit/303678350a7253c7bee9d6a3347ee1bcdf9cc177 https://github.com/openzfs/zfs/commit/303678350a7253c7bee9d6... You could argue otherwise and you would not be wrong since the codebase had not given this code inputs that would overrun the buffer. However, any case where you loop until the string is built needs a check on each iteration unless you can both prove that the buffer will always be long enough for any possible output and guarantee that future changes will preserve this property. The former is a pain (but doable) while the latter was infeasible, which is why the bounds checks were added. That said, something as innocuous as printing a floating value could print 308 characters where you only expected 10 or less: https://github.com/openzfs/zfs/commit/f954ea26a615cecc8573bb439482d9fd88019854 https://github.com/openzfs/zfs/commit/f954ea26a615cecc8573bb... Edge cases causing things to print beyond what was expected are a good reason why people should use bounds checks when building strings, since truncation is fail safe, while overrunning the buffer is fail dangerous. I am a fan of the silent truncation done in both patches, since in both cases, truncation is harmless. For cases where silent truncation is not harmless, you can use detect the truncation by checking the return value of `snprintf()` and react accordingly. It should be said that the scnprintf() function's return value does not permit truncation detection and `snprintf()` should be used if you want that. Note that I am the author of those two patches, so it is natural that I like the approach I used. They were written as part of an audit of the OpenZFS codebase I did when bug hunting for fun.

If they're all being inserted contiguously. Anyway that's a form of saying "I know by reasoning that none of these will be outside the bounds, so let's not check".

ryao 1y ago

It depends on the loop. Here are a bunch of loops that need bounds checks on every loop iteration: https://github.com/openzfs/zfs/commit/303678350a7253c7bee9d6a3347ee1bcdf9cc177 https://github.com/openzfs/zfs/commit/303678350a7253c7bee9d6... You could argue otherwise and you would not be wrong since the codebase had not given this code inputs that would overrun the buffer. However, any case where you loop until the string is built needs a check on each iteration unless you can both prove that the buffer will always be long enough for any possible output and guarantee that future changes will preserve this property. The former is a pain (but doable) while the latter was infeasible, which is why the bounds checks were added. That said, something as innocuous as printing a floating value could print 308 characters where you only expected 10 or less: https://github.com/openzfs/zfs/commit/f954ea26a615cecc8573bb439482d9fd88019854 https://github.com/openzfs/zfs/commit/f954ea26a615cecc8573bb... Edge cases causing things to print beyond what was expected are a good reason why people should use bounds checks when building strings, since truncation is fail safe, while overrunning the buffer is fail dangerous. I am a fan of the silent truncation done in both patches, since in both cases, truncation is harmless. For cases where silent truncation is not harmless, you can use detect the truncation by checking the return value of `snprintf()` and react accordingly. It should be said that the scnprintf() function's return value does not permit truncation detection and `snprintf()` should be used if you want that. Note that I am the author of those two patches, so it is natural that I like the approach I used. They were written as part of an audit of the OpenZFS codebase I did when bug hunting for fun.