Dan Hecht 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: (6 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-144) 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'. (if it were an out param, then we generally would use pointer rather than ref). 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 iter). http://gerrit.cloudera.org:8080/#/c/9626/2/be/src/runtime/lib-cache.cc File be/src/runtime/lib-cache.cc: http://gerrit.cloudera.org:8080/#/c/9626/2/be/src/runtime/lib-cache.cc@249 PS2, Line 249: lib_cache_.erase(entry_iter); Nit: let’s not add the extraneous parenthesis 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. 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 hold the lock (similar to when loading_status had an error, and the other error paths in this function)? or does the caller somehow distinguish these two error cases? (in which case the header comment for RefreshCacheEntry needs updating). Looking deeper... oh crazy, the caller does distinguish -- see line 293 in GetCacheEntry(); if (*entry == NULL) return status; // Set loading_status on the entry so that if another thread calls // GetCacheEntry() for this lib before this thread is able to acquire lock_ in // RemoveEntry(), it is able to return the same error. (*entry)->loading_status = status; This is preexisting weirdness, I guess, so maybe not worth mucking with but at the least noting in the header comments. -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 14 Mar 2018 23:42:14 +0000 Gerrit-HasComments: Yes
