Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
v8: Interrupts must not mask stack overflow.
Browse files Browse the repository at this point in the history 
Backport of https://codereview.chromium.org/339883002
  • Loading branch information
@indutny @tjfontaine
indutny authored and tjfontaine committed Jul 31, 2014
1 parent 1223caf commit 530af9c
Showing 1 changed file with 2 additions and 7 deletions.
9 changes: 2 additions & 7 deletions deps/v8/src/isolate.h
View file
Expand Up @@ -1392,14 +1392,9 @@ class StackLimitCheck BASE_EMBEDDED {
public:
explicit StackLimitCheck(Isolate* isolate) : isolate_(isolate) { }

bool HasOverflowed() const {
inline bool HasOverflowed() const {
StackGuard* stack_guard = isolate_->stack_guard();
// Stack has overflowed in C++ code only if stack pointer exceeds the C++
// stack guard and the limits are not set to interrupt values.
// TODO(214): Stack overflows are ignored if a interrupt is pending. This
// code should probably always use the initial C++ limit.
return (reinterpret_cast<uintptr_t>(this) < stack_guard->climit()) &&
stack_guard->IsStackOverflow();
return reinterpret_cast<uintptr_t>(this) < stack_guard->real_climit();
}
private:
Isolate* isolate_;
Expand Down

11 comments on commit 530af9c

@tunniclm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Hello! I am currently looking through both this change and the V8 change to understand the issue more deeply. I would really appreciate it if you could help me understand why only one of the file changes in the Chromium / V8 change set referenced was backported. I can see there are some differences in the base files for the others in the change set -- did you conclude that the version of V8 that Node is currently using doesn't have a problem in these files? Was there a change to a version of V8 between the Node's current version and the version that needed to be fixed that introduced problems in those other files?

Thanks!

@indutny
Copy link
Member Author

@indutny indutny commented on 530af9c Aug 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tunniclm technically there are other changes here, but they are not making node.js vulnerable to attacks. The changeset isn't applying cleanly to our current v8 version, so I decided to go with the easiest and the most observable fix that we could have.

@tunniclm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny I see. I was a little confused for a while there because (if I understand correctly) the change was made in https://codereview.chromium.org/11362007 "Correctly check for stack overflow even when interrupt is pending." rather than the linked change. Is that right? Or was there another reason to link to https://codereview.chromium.org/339883002 "Interrupts must not mask stack overflow." instead?

@indutny
Copy link
Member Author

@indutny indutny commented on 530af9c Aug 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tunniclm this is correct! I was unaware of that CL ticket, the one that this issue mentions is the one that v8 team has provided to me.

@tunniclm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Thanks for responses, they are very helpful!

When you look at https://codereview.chromium.org/11362007, it seems like there is nothing extra to backport. Would it be possible to confirm that 11362007 is the right ticket (and perhaps correct the blog post)? Or is there something more going on here that makes the original ticket 339883002 relevant - particularly regarding the security vulnerability?

@indutny
Copy link
Member Author

@indutny indutny commented on 530af9c Aug 5, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, cc @tjfontaine

@tunniclm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjfontaine @indutny I'm still interested to hear whether the ticket was cited incorrectly or whether I'm missing something. Thanks.

@indutny
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cited ticket contains the patch that you mention, so your ticket is more exact, but not more correct.

@tunniclm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Yes, sorry, "incorrectly" was the wrong word to use. Let me rephrase -- I'm still interested to hear whether there are aspects of 339883002 that are relevant to the V8 vulnerability that are not covered by https://codereview.chromium.org/11362007

@indutny
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no aspects of this vulnerability that are not covered by the ticket that you are mentioning.

@tunniclm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Thanks!

Please sign in to comment.