Take care with special characters in paths to configure scripts.

On all platforms, warn about their presence. On Windows, we should use
slashes (as opposed to backslashes) where possible, to avoid causing
things like #5386.

Closes #5386.

Cabal/ChangeLog.md

@@ -50,6 +50,11 @@
   * Recognize `powerpc64le` as architecture PPC64.
   * Cabal now deduplicates more `-I` and `-L` and flags to avoid `E2BIG`
     ([#5356](https://github.com/haskell/cabal/issues/5356)).
+  * With `build-type: configure`, avoid using backslashes to delimit
+    path components on Windows and warn about other unsafe characters
+    in the path to the source directory on all platforms
+    ([#5386](https://github.com/haskell/cabal/issues/5386)).
+
 ----
 
 ## 2.2.0.1 (current 2.2 development version)

Cabal/Distribution/Simple.hs

@@ -99,7 +99,7 @@ import System.Environment (getArgs, getProgName)
 import System.Directory   (removeFile, doesFileExist
                           ,doesDirectoryExist, removeDirectoryRecursive)
 import System.Exit                          (exitWith,ExitCode(..))
-import System.FilePath                      (searchPathSeparator, takeDirectory, (</>))
+import System.FilePath                      (searchPathSeparator, takeDirectory, (</>), splitDirectories, dropDrive)
 import Distribution.Compat.Directory        (makeAbsolute)
 import Distribution.Compat.Environment      (getEnvironment)
 import Distribution.Compat.GetShortPathName (getShortPathName)
@@ -726,6 +726,27 @@ runConfigureScript verbosity backwardsCompatHack flags lbi = do
   -- a way to pass its flags too
   configureFile <- makeAbsolute $
     fromMaybe "." (takeDirectory <$> cabalFilePath lbi) </> "configure"
+  -- autoconf is fussy about filenames, and has a set of forbidden
+  -- characters that can't appear in the build directory, etc:
+  -- https://www.gnu.org/software/autoconf/manual/autoconf.html#File-System-Conventions
+  --
+  -- This has caused hard-to-debug failures in the past (#5368), so we
+  -- detect some cases early and warn with a clear message. Windows's
+  -- use of backslashes is problematic here, so we'll switch to
+  -- slashes, but we do still want to fail on backslashes in POSIX
+  -- paths.
+  --
+  -- TODO: We don't check for colons, tildes or leading dashes. We
+  -- also should check the builddir's path, destdir, and all other
+  -- paths as well.
+  let configureFile' = intercalate "/" $ splitDirectories configureFile
+  for_ badAutoconfCharacters $ \(c, cname) ->
+    when (c `elem` dropDrive configureFile') $
+      warn verbosity $
+           "The path to the './configure' script, '" ++ configureFile'
+        ++ "', contains the character '" ++ [c] ++ "' (" ++ cname ++ ")."
+        ++ " This may cause the script to fail with an obscure error, or for"
+        ++ " building the package to fail later."
   let extraPath = fromNubList $ configProgramPathExtra flags
   let cflagsEnv = maybe (unwords ccFlags) (++ (" " ++ unwords ccFlags))
                   $ lookup "CFLAGS" env
@@ -734,7 +755,7 @@ runConfigureScript verbosity backwardsCompatHack flags lbi = do
                 ((intercalate spSep extraPath ++ spSep)++) $ lookup "PATH" env
       overEnv = ("CFLAGS", Just cflagsEnv) :
                 [("PATH", Just pathEnv) | not (null extraPath)]
-      args' = configureFile:args ++ ["CC=" ++ ccProgShort]
+      args' = configureFile':args ++ ["CC=" ++ ccProgShort]
       shProg = simpleProgram "sh"
       progDb = modifyProgramSearchPath
                (\p -> map ProgramSearchPathDir extraPath ++ p) emptyProgramDb
@@ -749,6 +770,30 @@ runConfigureScript verbosity backwardsCompatHack flags lbi = do
   where
     args = configureArgs backwardsCompatHack flags
 
+    badAutoconfCharacters =
+      [ (' ', "space")
+      , ('\t', "tab")
+      , ('\n', "newline")
+      , ('\0', "null")
+      , ('"', "double quote")
+      , ('#', "hash")
+      , ('$', "dollar sign")
+      , ('&', "ampersand")
+      , ('\'', "single quote")
+      , ('(', "left bracket")
+      , (')', "right bracket")
+      , ('*', "star")
+      , (';', "semicolon")
+      , ('<', "less-than sign")
+      , ('=', "equals sign")
+      , ('>', "greater-than sign")
+      , ('?', "question mark")
+      , ('[', "left square bracket")
+      , ('\\', "backslash")
+      , ('`', "backtick")
+      , ('|', "pipe")
+      ]
+
     notFoundMsg = "The package has a './configure' script. "
                ++ "If you are on Windows, This requires a "
                ++ "Unix compatibility toolchain such as MinGW+MSYS or Cygwin. "

Cabal/doc/developing-packages.rst

@@ -409,6 +409,15 @@ management. The need to remain compatible with automatic package
 management means that Cabal's conditional dependencies system is a bit
 less flexible than with the "./configure" approach.
 
+.. note::
+   `GNU autoconf places restrictions on paths, including the
+   path that the user builds a package from.
+   <https://www.gnu.org/software/autoconf/manual/autoconf.html#File-System-Conventions>`_
+   Package authors using ``build-type: configure`` should be aware of
+   these restrictions; because users may be unexpectedly constrained and
+   face mysterious errors, it is recommended that ``build-type: configure``
+   is only used where strictly necessary.
+
 Portability
 -----------
 

Cabal/doc/installing-packages.rst

@@ -470,6 +470,13 @@ passed the :option:`--with-hc-pkg`, :option:`--prefix`, :option:`--bindir`,
 :option:`--with-compiler` option is passed in a :option:`--with-hc-pkg` option
 and all options specified with :option:`--configure-option` are passed on.
 
+.. note::
+   `GNU autoconf places restrictions on paths, including the directory
+   that the package is built from.
+   <https://www.gnu.org/software/autoconf/manual/autoconf.html#File-System-Conventions>`_
+   The errors produced when this happens can be obscure; Cabal attempts to
+   detect and warn in this situation, but it is not perfect.
+
 In Cabal 2.0, support for a single positional argument was added to
 ``setup configure`` This makes Cabal configure a the specific component
 to be configured. Specified names can be qualified with ``lib:`` or

cabal-testsuite/PackageTests/AutoconfBadPaths/Setup.hs

+module Main (main) where
+
+import Distribution.Simple
+
+main :: IO ()
+main = defaultMainWithHooks autoconfUserHooks

cabal-testsuite/PackageTests/AutoconfBadPaths/cabal.out

+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo bar/configure', contains the character ' ' (space). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo	bar/configure', contains the character '	' (tab). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo
+bar/configure', contains the character '
+' (newline). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo"bar/configure', contains the character '"' (double quote). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo#bar/configure', contains the character '#' (hash). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo$bar/configure', contains the character '$' (dollar sign). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo&bar/configure', contains the character '&' (ampersand). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo'bar/configure', contains the character ''' (single quote). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo(bar/configure', contains the character '(' (left bracket). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo)bar/configure', contains the character ')' (right bracket). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo*bar/configure', contains the character '*' (star). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo;bar/configure', contains the character ';' (semicolon). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo<bar/configure', contains the character '<' (less-than sign). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo=bar/configure', contains the character '=' (equals sign). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo>bar/configure', contains the character '>' (greater-than sign). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo?bar/configure', contains the character '?' (question mark). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo[bar/configure', contains the character '[' (left square bracket). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo/bar/configure', contains the character '/' (backslash). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo`bar/configure', contains the character '`' (backtick). This may cause the script to fail with an obscure error, or for building the package to fail later.
+# cabal v1-configure
+Resolving dependencies...
+Configuring test-0...
+Warning: The path to the './configure' script, '/<ROOT>/cabal.dist/foo|bar/configure', contains the character '|' (pipe). This may cause the script to fail with an obscure error, or for building the package to fail later.

cabal-testsuite/PackageTests/AutoconfBadPaths/cabal.test.hs

+import Test.Cabal.Prelude
+import Data.Foldable (traverse_)
+main = cabalTest $ do
+  -- Test the forbidden characters except NUL. Reference:
+  -- https://www.gnu.org/software/autoconf/manual/autoconf.html#File-System-Conventions
+  -- Most of these are magic on Windows, so don't bother testing there.
+  --
+  -- Note: we bundle the configure script so no need to autoreconf
+  -- while building
+  skipIf =<< isWindows
+  traverse_ check
+    [ "foo bar"
+    , "foo\tbar"
+    , "foo\nbar"
+    , "foo\"bar"
+    , "foo#bar"
+    , "foo$bar"
+    , "foo&bar"
+    , "foo'bar"
+    , "foo(bar"
+    , "foo)bar"
+    , "foo*bar"
+    , "foo;bar"
+    , "foo<bar"
+    , "foo=bar"
+    , "foo>bar"
+    , "foo?bar"
+    , "foo[bar"
+    , "foo\\bar"
+    , "foo`bar"
+    , "foo|bar"
+    ]
+  where
+    -- 'cabal' from the prelude requires the command to succeed; we
+    -- don't mind if it fails, so long as we get the warning. This is
+    -- an inlined+specialised version of 'cabal' for v1-configure.
+    check dir = withSourceCopyDir dir $
+      defaultRecordMode RecordMarked $ do
+        recordHeader ["cabal", "v1-configure"]
+        env <- getTestEnv
+        let args =
+              [ "v1-configure"
+              , "-vverbose +markoutput +nowrap"
+              , "--builddir"
+              , testDistDir env
+              ]
+        configured_prog <- requireProgramM cabalProgram
+        r <- liftIO $ run (testVerbosity env)
+                      (Just (testCurrentDir env))
+                      (testEnvironment env)
+                      (programPath configured_prog)
+                      args
+        recordLog r

cabal-testsuite/PackageTests/AutoconfBadPaths/configure.ac

+AC_INIT([Test for autoconf brokenness], [0], [none@example.com], [test])
+
+AC_OUTPUT

cabal-testsuite/PackageTests/AutoconfBadPaths/test.cabal

+cabal-version: 2.2
+name: test
+version: 0
+build-type: Configure
+
+executable foo
+  main-is: Main.hs
+  default-language: Haskell2010

cabal-testsuite/PackageTests/Regression/T5386/Foo.hs

+{-# LANGUAGE CPP #-}
+module Foo where
+
+foo = FOO

cabal-testsuite/PackageTests/Regression/T5386/Main.hs

+main = return ()

cabal-testsuite/PackageTests/Regression/T5386/Setup.hs

+module Main (main) where
+
+import Distribution.Simple
+
+main :: IO ()
+main = defaultMainWithHooks autoconfUserHooks

cabal-testsuite/PackageTests/Regression/T5386/cabal.project

+packages:
+  ./

cabal-testsuite/PackageTests/Regression/T5386/cabal.test.hs

+import Test.Cabal.Prelude
+-- See #4332, dep solving output is not deterministic
+main = cabalTest . recordMode DoNotRecord $ withSourceCopyDir "9" $
+  -- Note: we bundle the configure script so no need to autoreconf while building
+  cabal "new-build" ["all"]

cabal-testsuite/PackageTests/Regression/T5386/configure.ac

+AC_INIT([Test for autoconf brokenness], [0], [none@example.com], [test])
+
+AC_CONFIG_FILES([test.buildinfo])
+
+AC_OUTPUT

cabal-testsuite/PackageTests/Regression/T5386/test.buildinfo.in

+ghc-options: -DFOO=42

cabal-testsuite/PackageTests/Regression/T5386/test.cabal

+cabal-version: 2.2
+name: test
+version: 0
+build-type: Configure
+
+executable foo
+  main-is: Main.hs
+  build-depends: base
+  default-language: Haskell2010
+
+library
+  exposed-modules: Foo
+  build-depends: base
+  default-language: Haskell2010

cabal-testsuite/Test/Cabal/Monad.hs

@@ -333,7 +333,8 @@ runTestM mode m = withSystemTempDirectory "cabal-testsuite" $ \tmp_dir -> do
                     testPlan = Nothing,
                     testRecordDefaultMode = DoNotRecord,
                     testRecordUserMode = Nothing,
-                    testRecordNormalizer = id
+                    testRecordNormalizer = id,
+                    testSourceCopyRelativeDir = "source"
                 }
     let go = do cleanup
                 r <- m
@@ -578,6 +579,9 @@ data TestEnv = TestEnv
     , testRecordUserMode :: Maybe RecordMode
     -- | Function to normalize recorded output
     , testRecordNormalizer :: String -> String
+    -- | Name of the subdirectory we copied the test's sources to,
+    -- relative to 'testSourceDir'
+    , testSourceCopyRelativeDir :: FilePath
     }
 
 testRecordMode :: TestEnv -> RecordMode
@@ -648,7 +652,7 @@ testKeysDir env = testWorkDir env </> "keys"
 
 -- | If 'withSourceCopy' is used, where the source files go.
 testSourceCopyDir :: TestEnv -> FilePath
-testSourceCopyDir env = testWorkDir env </> "source"
+testSourceCopyDir env = testWorkDir env </> testSourceCopyRelativeDir env
 
 -- | The user cabal directory
 testCabalDir :: TestEnv -> FilePath

cabal-testsuite/Test/Cabal/Prelude.hs

@@ -874,6 +874,8 @@ ghc' args = do
 -- This requires the test repository to be a Git checkout, because
 -- we use the Git metadata to figure out what files to copy into the
 -- hermetic copy.
+--
+-- Also see 'withSourceCopyDir'.
 withSourceCopy :: TestM a -> TestM a
 withSourceCopy m = do
     env <- getTestEnv
@@ -886,6 +888,21 @@ withSourceCopy m = do
             liftIO $ copyFile (cwd </> f) (dest </> f)
     withReaderT (\nenv -> nenv { testHaveSourceCopy = True }) m
 
+-- | If a test needs to modify or write out source files, it's
+-- necessary to make a hermetic copy of the source files to operate
+-- on.  This function arranges for this to be done in a subdirectory
+-- with a given name, so that tests that are sensitive to the path
+-- that they're running in (e.g., autoconf tests) can run.
+--
+-- This requires the test repository to be a Git checkout, because
+-- we use the Git metadata to figure out what files to copy into the
+-- hermetic copy.
+--
+-- Also see 'withSourceCopy'.
+withSourceCopyDir :: FilePath -> TestM a -> TestM a
+withSourceCopyDir dir =
+  withReaderT (\nenv -> nenv { testSourceCopyRelativeDir = dir }) . withSourceCopy
+
 -- | Look up the 'InstalledPackageId' of a package name.
 getIPID :: String -> TestM String
 getIPID pn = do