Add about overflow-checks flag in release mode #24
Conversation
There was a problem hiding this comment.
Thanks for adding some nuance @iglesias! Can you take a look at my comment?
| saturating, or wrapping operations. | ||
|
|
||
| .. rationale:: | ||
| :id: rat_kYiIiW8R2qD1 | ||
| :status: draft | ||
|
|
||
| In debug builds, Rust performs runtime checks for integer overflow and will panic if detected. | ||
| However, in release builds (with optimizations enabled), integer operations silently wrap | ||
| around on overflow, creating potential for silent failures and security vulnerabilities. | ||
| However, in release builds (with optimizations enabled), unless the flag overflow-checks is |
There was a problem hiding this comment.
While that will avoid overflow, I think that 'panic'ing may still be undesirable. I'd say if you added another sentence instead which described that silent wraps can be avoided with the overflow-checks flag, but we still panic, therefore we still advise being explicit that may work better. What do you think?
There was a problem hiding this comment.
Yeah, I think 'panic'inc is still undesirable in the context of this guideline's recommendation. I had considered whether it should be recommended adding it to the last sentence in the previous paragraph, then took a look at how the standard's library checked operation behaves (which is returning None) and decided not to. This change is about (probably a bit pedantically) note that the silent behavior isn't 100% implied by release / optimizations enabled. I will add another sentence here trying to clarify it.
There was a problem hiding this comment.
@iglesias -- could you update with the link that @AchimKriso shared for further context as well as the flag docs that you listed above?
There was a problem hiding this comment.
Sure. Links added in 50ad38f.
iglesias
force-pushed
the
overflow-checks
branch
from
05fce40 to
bf131d2
Compare
|
Here is some additional context from the language team that I think might be useful: |
There was a problem hiding this comment.
Hey thanks for being so patient on this one @iglesias! I think if you can add a couple of references in here then we can merge this one in.
| saturating, or wrapping operations. | ||
|
|
||
| .. rationale:: | ||
| :id: rat_kYiIiW8R2qD1 | ||
| :status: draft | ||
|
|
||
| In debug builds, Rust performs runtime checks for integer overflow and will panic if detected. | ||
| However, in release builds (with optimizations enabled), integer operations silently wrap | ||
| around on overflow, creating potential for silent failures and security vulnerabilities. | ||
| However, in release builds (with optimizations enabled), unless the flag overflow-checks is |
There was a problem hiding this comment.
@iglesias -- could you update with the link that @AchimKriso shared for further context as well as the flag docs that you listed above?
|
A check has failed here, but main seems to be good. I am going to rebase and force-push the commits. |
iglesias
force-pushed
the
overflow-checks
branch
from
50ad38f to
1b5d74f
Compare
There was a problem hiding this comment.
Thank you @iglesias for adding this context and the references!
Hi, I thought this can add a bit of information. Flag documentation.