[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

Once the Promise (of Status) is set in ProcessBuildInputAsync(),
the main thread in ProcessBuildInputAndOpenProbe() may proceed
to finish up the query and free RuntimeState. So, it's unsafe
to access the RuntimeState afterwards. Commit bb1c633 broke that
assumption (which is arguably fragile). This change fixes the
problem by adding a scope for the that counter to avoid the
use-after-free problem.

Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Reviewed-on: http://gerrit.cloudera.org:8080/5246
Reviewed-by: Dan Hecht 
Reviewed-by: Tim Armstrong 
Tested-by: Internal Jenkins
---
M be/src/exec/blocking-join-node.cc
1 file changed, 15 insertions(+), 11 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, but someone else must approve
  Dan Hecht: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/5246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5246/1/be/src/exec/blocking-join-node.cc
File be/src/exec/blocking-join-node.cc:

PS1, Line 167: // Please keep this as the last line in this function to avoid 
use-after-free problem.
 :   // Once 'status' is set, ProcessBuildInputAndProbe() will 
start running and 'states'
 :   // may have been freed after this line once the query 
completes. IMPALA-4532.
 :   // TODO: Make this less fragile.
 :   status->Set(s);
> Thanks Henry. Would you mind if I address this in a follow-up patch ? This 
Fine by me.


-- 
To view, visit http://gerrit.cloudera.org:8080/5246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/5246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1:

A better fix could be to make the main thread call Join() on the build thread.

-- 
To view, visit http://gerrit.cloudera.org:8080/5246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Michael Ho (Code Review)]

Michael Ho has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5246

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..

IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

Once the Promise (of Status) is set in ProcessBuildInputAsync(),
the main thread in ProcessBuildInputAndOpenProbe() may proceed
to finish up the query and free RuntimeState. So, it's unsafe
to access the RuntimeState afterwards. Commit bb1c633 broke that
assumption (which is arguably fragile). This change fixes the
problem by adding a scope for the that counter to avoid the
use-after-free problem.

Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
---
M be/src/exec/blocking-join-node.cc
1 file changed, 15 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/5246/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5246
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho