[impala] branch master updated: IMPALA-9571: Fix Impala crash from unexpected boost filesystem_error exception

[jo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git


The following commit(s) were added to refs/heads/master by this push:
     new 7606133  IMPALA-9571: Fix Impala crash from unexpected boost filesystem_error exception
7606133 is described below

commit 76061331781e67b90d7750abadfaf8b013dc64a5
Author: Joe McDonnell <jo...@cloudera.com>
AuthorDate: Thu Apr 2 15:19:42 2020 -0700

    IMPALA-9571: Fix Impala crash from unexpected boost filesystem_error exception
    
    The no-exception signature of boost::filesystem::remove_all() has a
    longstanding bug where it can throw an exception even for the
    no-exceptions interface. Boost recently added the "noexcept" specifier
    to the no-exception signature of remove_all(). When a "noexcept"
    function throws an exception, it causes the program to call std::terminate()
    which aborts the program. This makes the try/catch block around
    remove_all() useless.
    
    As a point fix, this switches FilesystemUtil to use the remove_all()
    signature that can throw exceptions. This code already had a try/catch
    block around remove_all(), so now it just uses that to get the error
    code.
    
    Newer versions of boost (1.63+) fix the underlying problem, so this
    code can be removed when we upgrade boost.
    
    Testing:
     - Ran core tests
     - Ran FilesystemUtil backend test
    
    Change-Id: I23016f37401604fc41fc99c0a056439a31333e3f
    Reviewed-on: http://gerrit.cloudera.org:8080/15640
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
    Reviewed-by: Csaba Ringhofer <cs...@cloudera.com>
---
 be/src/util/filesystem-util.cc | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/be/src/util/filesystem-util.cc b/be/src/util/filesystem-util.cc
index 5ec1755..83c3c90 100644
--- a/be/src/util/filesystem-util.cc
+++ b/be/src/util/filesystem-util.cc
@@ -87,10 +87,16 @@ Status FileSystemUtil::RemoveAndCreateDirectory(const string& directory) {
     // the check for existence above and the removal here. If the directory is removed in
     // this window, we may get "no_such_file_or_directory" error which is fine.
     //
-    // There is a bug in boost library (as of version 1.6) which may lead to unexpected
-    // exceptions even though we are using the no-exceptions interface. See IMPALA-2846.
+    // There is a bug in boost library (as of version 1.61) which may lead to unexpected
+    // exceptions from the no-exceptions interface. Even worse, the no-exceptions
+    // interface is marked as noexcept, so the unexpected exception will cause the
+    // program to immediately call std::terminate(). This renders the try/catch block
+    // useless. For now, this uses the interface that can throw exceptions and
+    // gets the errorcode from the exception. See IMPALA-2846 + IMPALA-9571.
+    // TODO: Newer boost has a fix for this bug, so this can be switched back after
+    // upgrading boost.
     try {
-      filesystem::remove_all(directory, errcode);
+      filesystem::remove_all(directory);
     } catch (filesystem::filesystem_error& e) {
       errcode = e.code();
     }
@@ -111,10 +117,11 @@ Status FileSystemUtil::RemoveAndCreateDirectory(const string& directory) {
 Status FileSystemUtil::RemovePaths(const vector<string>& directories) {
   for (int i = 0; i < directories.size(); ++i) {
     error_code errcode;
-    // There is a bug in boost library (as of version 1.6) which may lead to unexpected
-    // exceptions even though we are using the no-exceptions interface. See IMPALA-2846.
+    // This uses the boost remove_all() interface that can throw exceptions to avoid a
+    // bug in the implementation of the no-exceptions remove_all(). See comment in
+    // RemoveAndCreateDirectory() for more details.
     try {
-      filesystem::remove_all(directories[i], errcode);
+      filesystem::remove_all(directories[i]);
     } catch (filesystem::filesystem_error& e) {
       errcode = e.code();
     }