Allow bytestring-0.12 by Bodigrim · Pull Request #9241 · haskell/cabal

Bodigrim · 2023-09-09T19:31:17Z

Could this please be backported to whatever release is to accompany GHC 9.8?

Cf. https://gitlab.haskell.org/ghc/ghc/-/issues/23926#note_524594

Kleidukos · 2023-09-09T22:31:09Z

@Bodigrim Thanks a lot!

Bodigrim · 2023-09-10T22:08:28Z

Seems stuck on CI, could someone please unstuck it?

geekosaur · 2023-09-10T22:44:52Z

Did I make a mess?

/home/runner/work/cabal/cabal/dist-newstyle-validate-ghc-9.4.4/build/x86_64-linux/ghc-9.4.4/cabal-testsuite-3/build/global-autogen: checkForInteriorNuls: invalid argument (POSIX filepaths must not contain internal NUL octets.)

The diff from the rebase doesn't look like it could have caused this.

Bodigrim · 2023-09-11T00:00:03Z

No, it's not you, it's a new unix-2.8.2.0: https://hackage.haskell.org/package/unix-2.8.2.0/changelog
This is unrelated to bytestring-0.12, but also needs fixing before release for GHC 9.8: if something inside Cabal test suite (rogue QuickCheck instance?) generates file names with \NUL, we are in trouble. (Such filenames used to be truncated silently before)

Mikolaj · 2023-09-11T09:08:56Z

@geekosaur: I can't see this NUL error message in CI logs. Where is it? How do we reproduce?

Mikolaj · 2023-09-11T09:21:44Z

If this is reproduced, we need a new ticket for it and I'm sure @Kleidukos would want to block 3.10.2.0 on it.

geekosaur · 2023-09-11T16:18:08Z

Configuring test suite 'check-tests' for Cabal-tests-3..
Preprocessing test suite 'check-tests' for Cabal-tests-3..
Building test suite 'check-tests' for Cabal-tests-3..
[1 of 1] Compiling Main             ( tests/CheckTests.hs, /mnt/data1/cabal-source/dist-newstyle-validate-ghc-9.4.7/build/x86_64-linux/ghc-9.4.7/Cabal-tests-3/t/check-tests/build/check-tests/check-tests-tmp/Main.o )

tests/CheckTests.hs:6:1: warning: [-Wunused-imports]
    The import of ‘Test.Tasty.ExpectedFailure’ is redundant
      except perhaps to import instances from ‘Test.Tasty.ExpectedFailure’
    To import instances alone, use: import Test.Tasty.ExpectedFailure()
  |
6 | import Test.Tasty.ExpectedFailure
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[2 of 2] Linking /mnt/data1/cabal-source/dist-newstyle-validate-ghc-9.4.7/build/x86_64-linux/ghc-9.4.7/Cabal-tests-3/t/check-tests/build/check-tests/check-tests
[1 of 2] Compiling Main             ( /mnt/data1/cabal-source/dist-newstyle-validate-ghc-9.4.7/build/x86_64-linux/ghc-9.4.7/cabal-testsuite-3/setup/setup.hs, /mnt/data1/cabal-source/dist-newstyle-validate-ghc-9.4.7/build/x86_64-linux/ghc-9.4.7/cabal-testsuite-3/setup/Main.o )
[2 of 2] Linking /mnt/data1/cabal-source/dist-newstyle-validate-ghc-9.4.7/build/x86_64-linux/ghc-9.4.7/cabal-testsuite-3/setup/setup
Configuring cabal-testsuite-3...
/usr/bin/tar: getPermissions:checkForInteriorNuls: invalid argument (POSIX filepaths must not contain internal NUL octets.)
Error: cabal: Failed to build cabal-testsuite-3. The failure occurred during
the configure step.

geekosaur · 2023-09-11T16:42:45Z

Am I misreading the above, or is this happening while verifying permissions (presumably exec) on /usr/bin/tar? I tried looking at that but saw nothing obvious. It does make me wonder if something is spuriously NUL-terminating paths, though.

gbaz · 2023-09-11T18:17:01Z

I concluded there's an off-by-one logic bug somewhere between filepath, unix-2.8.2.0, and bytestring's short-bytestring interpretation.

This gist shows it. https://gist.github.com/gbaz/f068beb5da3acab08444452b1a421fd8

For cabal I suggest we enforce unix < 2.8.2.0 for now and force upstream to sort it out.

gbaz · 2023-09-11T18:28:40Z

@hasufell see above ^^

Mikolaj · 2023-09-11T23:40:21Z

@Bodigrim: would you alert the Upstream?

@Kleidukos: do you approve unix < 2.8.2.0 as proposed by @gbaz? If so, perhaps we could add it as yet another commit to this PR?

Anything else we are missing?

geekosaur · 2023-09-11T23:42:27Z

I don't think we've sorted out which upstream to notify yet. It could be bytestring if it's an off-by-one in length, or it could be a bug in encodeFP from filepath.

hasufell · 2023-09-12T00:40:54Z

This looks like something worse than off by one:

ghci> :set -XOverloadedStrings
ghci> import qualified Data.ByteString.Short as BSS
ghci> import Data.ByteString.Internal (c_strlen)
ghci> BSS.useAsCStringLen "ScriptEnv0.hs" $ \(ptr, len) -> c_strlen ptr >>= \clen -> print (clen, len)
(13,13)
ghci> BSS.useAsCStringLen "ScriptEnv0.hs" $ \(ptr, len) -> c_strlen ptr >>= \clen -> print (clen, len)
(38,13)
ghci> BSS.useAsCStringLen "ScriptEnv0.hs" $ \(ptr, len) -> c_strlen ptr >>= \clen -> print (clen, len)
(29,13)
ghci> BSS.useAsCStringLen "ScriptEnv0.hs" $ \(ptr, len) -> c_strlen ptr >>= \clen -> print (clen, len)
(38,13)

hasufell · 2023-09-12T00:45:51Z

Actually it's not. You're just using c_strlen wrong. useAsCStringLen does not null-terminate the CString, so you're relying on the memory location to be sane: https://hackage.haskell.org/package/bytestring-0.12.0.2/docs/src/Data.ByteString.Short.Internal.html#useAsCStringLen

This is not a bug.

geekosaur · 2023-09-12T00:47:14Z

We're not the ones using it; that code is buried in directory.

hasufell · 2023-09-12T00:47:29Z

So where exactly does this issue surface? Do you use c_strlen in your test? Does something in filepath/unix use it wrongly? Which codepath is failing?

hasufell · 2023-09-12T00:49:55Z

Ok, I found it: https://github.com/haskell/unix/blob/fadc6e1b07c212a388bc08f1f8b32873edf9d164/System/Posix/PosixPath/FilePath.hsc#L149-L166

geekosaur · 2023-09-12T00:51:05Z

Right, I just landed there after retracing my steps earlier this afternoon, which started with directory.

hasufell · 2023-09-12T01:11:00Z

haskell/unix#294

hasufell · 2023-09-12T01:13:13Z

Please try the fix by adjusting your cabal.project pointing it to commit 2f4b1cf136c09c176a83617d5a20fc5c4b2d5264 in unix.

Mikolaj · 2023-09-12T07:54:13Z

@Bodigrim says "In the meantime I've revised unix-2.8.2.0 with base < 0" so we can probably resume the release of 3.10.2.0. Let me restart the failing tests on CI.

Mikolaj · 2023-09-14T01:03:38Z

@mergify backport 3.10

mergify · 2023-09-14T01:03:41Z

backport 3.10

✅ Backports have been created

Details

#9257 Allow bytestring-0.12 (backport #9241) has been created for branch 3.10 but encountered conflicts

Co-authored-by: Bodigrim <[email protected]> Co-authored-by: Hécate Moonlight <[email protected]>

geekosaur approved these changes Sep 9, 2023

View reviewed changes

Kleidukos approved these changes Sep 9, 2023

View reviewed changes

Kleidukos added merge me Tell Mergify Bot to merge merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Sep 9, 2023

Allow bytestring-0.12

869e0e5

geekosaur force-pushed the bytestring-0.12 branch from 98c74be to 869e0e5 Compare September 10, 2023 22:10

geekosaur mentioned this pull request Sep 11, 2023

Put hie files in subdirectory of extra-compilation-artifacts #9244

Merged

4 tasks

Bodigrim mentioned this pull request Sep 11, 2023

Fails to build with GHC 9.6.2 and the latest hackage index haskell/entropy#83

Closed

hasufell mentioned this pull request Sep 12, 2023

Fix withFilePath haskell/unix#294

Merged

mergify Bot merged commit 992765f into master Sep 12, 2023

mergify Bot deleted the bytestring-0.12 branch September 12, 2023 10:24

geekosaur mentioned this pull request Sep 12, 2023

QuickCheck PathName function may produce bad paths on Windows #9253

Closed

bgamari mentioned this pull request Sep 13, 2023

Memory unsafety in withFilePath in 2.8.2.0 haskell/unix#295

Closed

mergify Bot mentioned this pull request Sep 14, 2023

Allow bytestring-0.12 (backport #9241) #9257

Merged

Kleidukos added a commit that referenced this pull request Sep 14, 2023

Allow bytestring-0.12 (backport #9241) (#9257)

390dc1c

Co-authored-by: Bodigrim <[email protected]> Co-authored-by: Hécate Moonlight <[email protected]>

Conversation

Bodigrim commented Sep 9, 2023

Uh oh!

Kleidukos commented Sep 9, 2023

Uh oh!

Bodigrim commented Sep 10, 2023

Uh oh!

geekosaur commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bodigrim commented Sep 11, 2023

Uh oh!

Mikolaj commented Sep 11, 2023

Uh oh!

Mikolaj commented Sep 11, 2023

Uh oh!

geekosaur commented Sep 11, 2023

Uh oh!

geekosaur commented Sep 11, 2023

Uh oh!

gbaz commented Sep 11, 2023

Uh oh!

gbaz commented Sep 11, 2023

Uh oh!

Mikolaj commented Sep 11, 2023

Uh oh!

geekosaur commented Sep 11, 2023

Uh oh!

hasufell commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasufell commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geekosaur commented Sep 12, 2023

Uh oh!

hasufell commented Sep 12, 2023

Uh oh!

hasufell commented Sep 12, 2023

Uh oh!

geekosaur commented Sep 12, 2023

Uh oh!

hasufell commented Sep 12, 2023

Uh oh!

hasufell commented Sep 12, 2023

Uh oh!

Mikolaj commented Sep 12, 2023

Uh oh!

Mikolaj commented Sep 14, 2023

Uh oh!

mergify Bot commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

geekosaur commented Sep 10, 2023 •

edited

Loading

hasufell commented Sep 12, 2023 •

edited

Loading

hasufell commented Sep 12, 2023 •

edited

Loading

mergify Bot commented Sep 14, 2023 •

edited

Loading