Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h File be/src/runtime/lib-cache.h: http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@157 PS3, Line 157: /// refreshed. The cache lock is required to be held prior to calling this method. > also mention the entry locking behavior (could copy sentence on line 143-14 Done http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@159 PS3, Line 159: LibMap::iterator& iter > is that modified (i.e. returned)? assuming no, it should be 'const ref'. (i Done http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.h@160 PS3, Line 160: LibCacheEntry** entry); > it'd be nice to put all the "out params" last (i.e. put entry_lock after it Done http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc File be/src/runtime/lib-cache.cc: http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc@320 PS3, Line 320: cache lock is : // *not* held > Let's add the "why" -- something about loading the entry being expensive. Done http://gerrit.cloudera.org:8080/#/c/9626/3/be/src/runtime/lib-cache.cc@394 PS3, Line 394: RETURN_IF_ERROR((*entry)->copy_file_status); > the error case here seems wrong. shouldn't we set *entry to nullptr and not right. the status is saved in the entry for the error case. added some clarifying comments since, yes, this is bit convoluted. -- To view, visit http://gerrit.cloudera.org:8080/9626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e Gerrit-Change-Number: 9626 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 15 Mar 2018 01:36:47 +0000 Gerrit-HasComments: Yes
