signed integer overflow in S_study_chunk #17848
Conversation
delta and pos_delta may hold OPTIMIZE_INFTY (SSize_t_MAX) to represent infinity; Fixes: Perl#17847
The expression we're about to add to data->pos_delta in this part of study_chunk() can be both positive or negative; however while we apply an overflow check to avoid exceeding OPTIMIZE_INFTY (used to represent infinity), we'll happily subtract from it when the expression is negative, making it no longer infinite. Fixes: Perl#17847
|
Should this be in 5.32.0? |
|
@khwilliamson objected to the third commit "I think that subtracting a finite value from infinity should be infinity, so I think I don't like your third patch." I think that objection may be because the commit message is not clear enough - when it says ".. we'll happily subtract from it when the expression is He also suggested that these checks should be using REG_INFTY (or at worst a new symbol such as OPTIMIZE_INFTY) rather than SSize_t_MAX, and @lightsey appears to have implemented that in these versions of the commits. I'm unsure about this, and would have preferred such changes to have a separate commit with a properly attributed author. I don't think I'm happy about the new version using OPTIMIZE_INFTY - and TBH I'm unsure about overflow handling in C more generally - because I'm concerned primarily about physical overflow of a particular datatype leading to wraparound (or undefined behaviour). What I really want is a way to have addition (or other arithmetic) safe for the actual datatype of the variable, so that if the type changes we will automatically switch to using the new relevant MAX value. But I don't know how to achieve that, and in any case we should discuss the philosophy of infinities somewhere distinct from this pull request. |
I'd say no (expressing opinion, not veto). Analysis suggested that if we hit this it fails in a safe way, so there's nothing to justify a late addition to the release. |
|
On 6/9/20 5:11 AM, Hugo van der Sanden wrote:
@khwilliamson <https://github.com/khwilliamson> objected to the third
commit "I think that subtracting a finite value from infinity should be
infinity, so I think I don't like your third patch." I think that
objection may be because the commit message is not clear enough - when
it says ".. we'll happily subtract from it when the expression is
negative, making it no longer infinite" I was describing the wrong state
of affairs this commit was fixing, not the result after the commit. I
believe this commit does correctly leave it acting to keep infinite
values infinite.
Hugo answered my concerns. I guess I should have said something rather
than just go silent.
He also suggested that these checks should be using REG_INFTY (or at
worst a new symbol such as OPTIMIZE_INFTY) rather than SSize_t_MAX, and
@lightsey <https://github.com/lightsey> appears to have implemented that
in these versions of the commits. I'm unsure about this, and would have
preferred such changes to have a separate commit with a properly
attributed author.
I don't think I'm happy about the new version using OPTIMIZE_INFTY - and
TBH I'm unsure about overflow handling in C more generally - because I'm
concerned primarily about physical overflow of a particular datatype
leading to wraparound (or undefined behaviour). What I really want is a
way to have addition (or other arithmetic) safe for the actual datatype
of the variable, so that if the type changes we will automatically
switch to using the new relevant MAX value. But I don't know how to
achieve that, and in any case we should discuss the philosophy of
infinities somewhere distinct from this pull request.
In became unsure that OPTIMIZE_INFTY was a good idea.
…
—
|
|
On 6/9/20 5:15 AM, Hugo van der Sanden wrote:
Should this be in 5.32.0?
I'd say no (expressing opinion, not veto). Analysis suggested that if we
hit this it fails in a safe way, so there's nothing to justify a late
addition to the release.
++
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17848 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2DHYLZOSJXE4YKTC5QVLRVYKW3ANCNFSM4NY2WJPQ>.
|
|
The OPTIMIZE_INFTY does look odd in combination with a cast to SSize_t to silence the compiler warnings. When trying to sync up the original change set against the OPTIMIZE_INFTY change I debated whether it would make more sense to use SSize_t_MAX for the comparisons or to case the SSize_t side of the comparison to UV. I apologize if my changes to the second commit seem more like a rewrite than a rebase. That wasn't my intent. The recent changes in the regex engine made it difficult to rebase the original fix without modifying it. At any rate, if @hvds wants to address this in a different fashion down the road, it'd make sense to close out this pull request. |
|
also @khwilliamson it's not clear if you are ok with this. |
toddr
added
Ready-to-merge
and removed
do not merge
|
I'd like to look through it again in light of the various comments, I'll try to do that within 24 hours. Putting together comments from me, @khwilliamson and @lightsey I'd probably go with recreating it more like the original, using SSize_t_MAX rather than OPTIMIZE_INFTY; but it's been a while, so I also want to review the whole of the changes again and make sure I still understand what I'm doing. |
|
Ok, on reviewing the current state of affairs, I think SSize_t_MAX no longer makes sense, so I've updated my patches close to what @lightsey had over latest blead; I've pushed this as branch gh17847 for consideration. I don't consider either SSize_t_MAX or OPTIMIZE_INFTY to be ideal - neither actually usefully describes what sort of quantity or limit we're dealing with. I'd rather we decided what these types of quantities should be, typedefed something to that, and then used an associated MAX define as our infinity. But then make all variables representing those quantities of the relevant type so that we don't end up trying to add a UV to a SSize_t. In particular I'm nervous that at some point someone will try to set |
I don't think A UV or IV is large enough to hold a pointer. |
|
This PR should have been closed a while ago, the issue was resolved by merging 3 commits ending with 8227b2b. |
This is a slightly modified version of the commits @hvds put together for #17847 in the security team issue tracker. I changed the comparisons in the second commit slightly to avoid compiler warnings about comparisons between signed and unsigned integers.