impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3854: Fix use-after-free in HdfsTextScanner::Close()
Date Sat, 16 Jul 2016 01:13:32 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-3854: Fix use-after-free in HdfsTextScanner::Close()
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3630/3/be/src/exec/scanner-context.cc
File be/src/exec/scanner-context.cc:

Line 69:   std::unique_ptr<Stream> stream(new Stream(this));
> I think it may be better to have a regular pointer here to avoid implicit d
As discussed offline, the unique_ptr here shows the ownership of the stream by the ScannerContext.
Whether we should use implicit destruction is a bit beyond this change IMHO. To certain extent,
it's the same as asking whether we should use scoped_ptr (or the like) in our code. FWIW,
we have plenty of places in our code which relies on implicit destruction to free resources
(e.g. RuntimeState's object pool, various classes' scoped_ptr). We don't always explicitly
destroy those resources in the destructors. Whether those are good practices can be discussed
but I prefer to stick to the existing convention in this patch for consistency.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88f6285563ff669ae215af22ed2d45e5398adae
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message