Ignore invalid Unicode in pkg-config descriptions #9609

tomsmeding · 2024-01-11T10:18:28Z

Previously, if any of the pkg-config packages on the system had invalid Unicode in their description fields (like the Intel vpl package has at the time of writing, 2024-01-11, see #9608), cabal would crash because it tried to interpret the entire pkg-config --list-all output as Unicode.

This change, as suggested by gbaz in #9608 (comment), switches to using a lazy ByteString for reading in the output, splitting on the first space in byte land, and then parsing only the package name to a String.

For further future-proofing, package names that don't parse as valid Unicode don't crash Cabal, but are instead ignored.

Fixes #9608.

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary. (unnecessary, bug fix)
Manual QA notes have been included. (unnecessary, no CLI change)
Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

alt-romes

Good work!

alt-romes · 2024-01-11T10:46:35Z

cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs

-        pkgList <- lines <$> getProgramOutput verbosity pkgConfig ["--list-all"]
+        -- To prevent malformed Unicode in the descriptions from crashing cabal,
+        -- read without interpreting any encoding first. (#9608)
+        pkgList <- LBS.split 10 <$> getProgramInvocationLBS verbosity (programInvocation pkgConfig ["--list-all"])


I suggest we avoid the literal 10, it confused me initially

Suggested change

pkgList <- LBS.split 10 <$> getProgramInvocationLBS verbosity (programInvocation pkgConfig ["--list-all"])

pkgList <- LBS.split (fromEnum '\n') <$> getProgramInvocationLBS verbosity (programInvocation pkgConfig ["--list-all"])

It should be a Word8, so that'd become fromIntegral (ord '\n') which is quite verbose. But fair point, maybe one shouldn't assume extensive familiarity with ASCII from Cabal contributors :D

I suppose you could also just add -- 10 is ASCII for '\n', or -- split on newlines (10 == fromEnum '\n')

I made it the verbose version, and just added a newline ;)

Mikolaj

LGTM

mpickering · 2024-01-11T10:45:09Z

cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs

        -- The output of @pkg-config --list-all@ also includes a description
        -- for each package, which we do not need.
-        let pkgNames = map (takeWhile (not . isSpace)) pkgList
+        let pkgNamesLBS = map (LBS.takeWhile (not . isSpace . chr . fromIntegral)) pkgList


How does this work if you have multi-point unicode characters?

It will not! You are correct, isSpace also recognises non-breaking spaces (U+00A0) which, when considered mod 256, have the high bit set and can thus occur in UTF-8 text as part of a multi-byte unicode codepoint. I had naively expected there to be no space characters in the Unicode range between 127 and 256, which would have made this work due to the UTF-8 encoding.

What do you suggest? Something like not . isAsciiSpace where isAsciiSpace n = n `elem` map (fromIntegral . ord) " \t"?

mpickering · 2024-01-11T10:45:16Z

cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs

-        pkgList <- lines <$> getProgramOutput verbosity pkgConfig ["--list-all"]
+        -- To prevent malformed Unicode in the descriptions from crashing cabal,
+        -- read without interpreting any encoding first. (#9608)
+        pkgList <- LBS.split 10 <$> getProgramInvocationLBS verbosity (programInvocation pkgConfig ["--list-all"])


What is this magic number 10?

That's an ASCII newline. This has been changed on alt-romes' suggestion to fromIntegral (ord '\n') in a later commit.

That's already spotted by @alt-romes and fixed by the author.

Mikolaj · 2024-01-11T11:18:55Z

BTW, would it be possible to add a small test with sample invalid characters preceded by valid ones?

michaelpj · 2024-01-11T11:33:52Z

cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs

-        let pkgNames = map (takeWhile (not . isSpace)) pkgList
+        let pkgNamesLBS = map (LBS.takeWhile (not . isSpace . chr . fromIntegral)) pkgList
+        -- Now decode as UTF8 and convert to String, dropping any that fail decoding.
+        let pkgNames = rights $ map (fmap T.unpack . T.decodeUtf8' . LBS.toStrict) pkgNamesLBS


Dropping things without giving any output saying that you're doing that seems likely to lead to very hard-to-debug issues later. Can we log (at high verbosity) saying that we're ignoring an entry because it has invalid unicode?

Thanks! You're right. I'm now logging something at Verbose level, which seems to be -v2 (but not 100% sure). The test also checks this.

Mikolaj · 2024-01-11T13:49:39Z

CI:

src/Distribution/Solver/Types/PkgConfigDb.hs:87:55: error:
    Not in scope: ‘T.decodeUtf8Lenient’
    Neither ‘Data.Text’ nor ‘Data.Text.Encoding’ exports ‘decodeUtf8Lenient’.
   |
87 |                             Left _ -> Left (T.unpack (T.decodeUtf8Lenient sbsname))

for a 9.2.7 buid.

Mikolaj · 2024-01-12T10:42:44Z

CI still grumpy. This time likely related to the changes themselves.

tomsmeding · 2024-01-12T12:05:02Z

@Mikolaj My latest commit fixes the failure in PackageTests/ExtraProgPath that I saw in the CI. It's an ugly fix, though, and you (plural) will probably have opinions on how to do this in cabal. Feel free to edit however you wish; from now on I'll have very little time work work on this until Monday 2024-01-22.

EDIT: Just to summarise, the original getProgramOutput throw an IO exception when the invoked program (pkg-config here) returned a non-zero exit code, but getProgramOutputLBS has been upgraded to use CabalExceptions and thus its non-zero-exit-code exception did not get caught by the handler at the top of readPkgConfigDb. My fix is to convert it back to an IO exception. You will probably want to do something different here.

Mikolaj · 2024-01-15T11:30:09Z

Let me trigger CI again for the unlikely case the failure was a fluke...

Edit: it broke the same.

Mikolaj · 2024-01-15T11:30:18Z

@mergify rebase

Previously, if any of the pkg-config packages on the system had invalid Unicode in their description fields (like the Intel vpl package has at the time of writing, 2024-01-11, see haskell#9608), cabal would crash because it tried to interpret the entire `pkg-config --list-all` output as Unicode. This change, as suggested by gbaz in haskell#9608 (comment) switches to using a lazy ByteString for reading in the output, splitting on the first space in byte land, and then parsing only the package _name_ to a String. For further future-proofing, package names that don't parse as valid Unicode don't crash Cabal, but are instead ignored.

mergify · 2024-01-15T11:30:28Z

rebase

✅ Branch has been successfully rebased

mpickering · 2024-01-15T11:37:31Z

cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs

-        let pkgNames = map (takeWhile (not . isSpace)) pkgList
+        -- To prevent malformed Unicode in the descriptions from crashing cabal,
+        -- read without interpreting any encoding first. (#9608)
+        pkgList <- LBS.split (fromIntegral (ord '\n')) <$>


This implementation doesn't seem very nice to me, it at least should be abstracted into it's own function.

How about using getProgramInvocationsIODataAndErrors? Which doesn't crash when the exitCode is not ExitSuccess

Could you use a parser abstraction to parse the input (you can use Parsec interface in cabal-install-solver)

In any case I don't think all this low level logic should be inlined here, so how about a new top level function getPkgConfigPkgList to encapsulate it all.

I think using parsec to split a package list on newlines is extreme overkill. Additionally, I don't think this is a lot of logic to inline, just well documented logic. And finally its actually the bulk of the function -- the existing top level function is already the encapsulation.

This is to appease shellcheck

review is cosmentic and blocking merge

Mikolaj · 2024-01-24T20:38:40Z

Let me restart the CI. The current failure starts with the following on macos:

Running: /Users/runner/work/cabal/cabal/cabal-testsuite/PackageTests/PkgConfigParse/./pkg-config --list-all
Failed to query pkg-config, Cabal will continue without solving for pkg-config
constraints:
/Users/runner/work/cabal/cabal/cabal-testsuite/PackageTests/PkgConfigParse/./pkg-config:
createProcess: posix_spawnp: does not exist (No such file or directory)

MacOS doesn't have /usr/bin/sh, and /bin/sh is the standard (for a POSIX shell) anyway

Mikolaj · 2024-01-25T06:11:24Z

@mergify backport 3.10

mergify · 2024-01-25T06:11:28Z

backport 3.10

✅ Backports have been created

#9658 Ignore invalid Unicode in pkg-config descriptions (backport #9609) has been created for branch 3.10 but encountered conflicts

* Ignore invalid Unicode in pkg-config descriptions Previously, if any of the pkg-config packages on the system had invalid Unicode in their description fields (like the Intel vpl package has at the time of writing, 2024-01-11, see #9608), cabal would crash because it tried to interpret the entire `pkg-config --list-all` output as Unicode. This change, as suggested by gbaz in #9608 (comment) switches to using a lazy ByteString for reading in the output, splitting on the first space in byte land, and then parsing only the package _name_ to a String. For further future-proofing, package names that don't parse as valid Unicode don't crash Cabal, but are instead ignored. * Add changelog entry * cabal-install-solver: Add bounds on 'text' * No literal ASCII values, use 'ord' * Address review comments re invalid unicode from pkg-config * Add test for invalid unicode from pkg-config * Compatibility with text-1.2.5.0 * Align imports * Handle different exception type * Use only POSIX shell syntax * Add invalid-input handler in pkg-config shim This is to appease shellcheck * Actually implement all required stuff in the pkg-config shim * Less exception dance * Fix shebang lines MacOS doesn't have /usr/bin/sh, and /bin/sh is the standard (for a POSIX shell) anyway * Don't expect a particular representation of invalid characters --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 0b34b4e) # Conflicts: # Cabal/src/Distribution/Simple/Program/Run.hs

tomsmeding · 2024-01-25T08:55:05Z

Thanks everyone for the reviews! And also thanks everyone for the help, in particular @Mikolaj :)

* Ignore invalid Unicode in pkg-config descriptions Previously, if any of the pkg-config packages on the system had invalid Unicode in their description fields (like the Intel vpl package has at the time of writing, 2024-01-11, see #9608), cabal would crash because it tried to interpret the entire `pkg-config --list-all` output as Unicode. This change, as suggested by gbaz in #9608 (comment) switches to using a lazy ByteString for reading in the output, splitting on the first space in byte land, and then parsing only the package _name_ to a String. For further future-proofing, package names that don't parse as valid Unicode don't crash Cabal, but are instead ignored. * Add changelog entry * cabal-install-solver: Add bounds on 'text' * No literal ASCII values, use 'ord' * Address review comments re invalid unicode from pkg-config * Add test for invalid unicode from pkg-config * Compatibility with text-1.2.5.0 * Align imports * Handle different exception type * Use only POSIX shell syntax * Add invalid-input handler in pkg-config shim This is to appease shellcheck * Actually implement all required stuff in the pkg-config shim * Less exception dance * Fix shebang lines MacOS doesn't have /usr/bin/sh, and /bin/sh is the standard (for a POSIX shell) anyway * Don't expect a particular representation of invalid characters --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 0b34b4e)

Ignore invalid Unicode in pkg-config descriptions (backport #9609)

* Ignore invalid Unicode in pkg-config descriptions Previously, if any of the pkg-config packages on the system had invalid Unicode in their description fields (like the Intel vpl package has at the time of writing, 2024-01-11, see haskell#9608), cabal would crash because it tried to interpret the entire `pkg-config --list-all` output as Unicode. This change, as suggested by gbaz in haskell#9608 (comment) switches to using a lazy ByteString for reading in the output, splitting on the first space in byte land, and then parsing only the package _name_ to a String. For further future-proofing, package names that don't parse as valid Unicode don't crash Cabal, but are instead ignored. * Add changelog entry * cabal-install-solver: Add bounds on 'text' * No literal ASCII values, use 'ord' * Address review comments re invalid unicode from pkg-config * Add test for invalid unicode from pkg-config * Compatibility with text-1.2.5.0 * Align imports * Handle different exception type * Use only POSIX shell syntax * Add invalid-input handler in pkg-config shim This is to appease shellcheck * Actually implement all required stuff in the pkg-config shim * Less exception dance * Fix shebang lines MacOS doesn't have /usr/bin/sh, and /bin/sh is the standard (for a POSIX shell) anyway * Don't expect a particular representation of invalid characters --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Mikolaj added the attention: needs-review label Jan 11, 2024

Mikolaj requested a review from gbaz January 11, 2024 10:36

alt-romes approved these changes Jan 11, 2024

View reviewed changes

Mikolaj approved these changes Jan 11, 2024

View reviewed changes

mpickering reviewed Jan 11, 2024

View reviewed changes

michaelpj reviewed Jan 11, 2024

View reviewed changes

tomsmeding requested a review from Mikolaj January 11, 2024 15:16

tomsmeding added the squash+merge me Tell Mergify Bot to squash-merge label Jan 11, 2024

geekosaur approved these changes Jan 12, 2024

View reviewed changes

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 14, 2024

tomsmeding added 9 commits January 15, 2024 11:30

Add changelog entry

fdd3e24

cabal-install-solver: Add bounds on 'text'

bb9f9da

No literal ASCII values, use 'ord'

6d179f6

Address review comments re invalid unicode from pkg-config

6569780

Add test for invalid unicode from pkg-config

2ec89dc

Compatibility with text-1.2.5.0

7d0d59f

Align imports

e724111

Handle different exception type

2900ecd

Mikolaj force-pushed the pkg-config-badunicode branch from 893fa00 to 2900ecd Compare January 15, 2024 11:30

mpickering previously requested changes Jan 15, 2024

View reviewed changes

Mikolaj removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 15, 2024

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 17, 2024

tomsmeding added 3 commits January 23, 2024 21:36

Use only POSIX shell syntax

89a2f05

Add invalid-input handler in pkg-config shim

28f7082

This is to appease shellcheck

Actually implement all required stuff in the pkg-config shim

ab8870a

gbaz approved these changes Jan 23, 2024

View reviewed changes

Less exception dance

8057eb3

tomsmeding mentioned this pull request Jan 23, 2024

Cabal 3.10.2.1 cannot parse pkg-config: hGetContents: invalid argument (invalid byte sequence) #9608

Closed

tomsmeding added 2 commits January 24, 2024 21:42

Fix shebang lines

fd66f0c

MacOS doesn't have /usr/bin/sh, and /bin/sh is the standard (for a POSIX shell) anyway

Don't expect a particular representation of invalid characters

16b2fc8

Mikolaj mentioned this pull request Jan 24, 2024

Tests UpdateIndexState and ExtraProgPath are weak/fragile #9657

Open

Merge branch 'master' into pkg-config-badunicode

aeedcd0

mergify bot merged commit 0b34b4e into haskell:master Jan 25, 2024
50 checks passed

mergify bot mentioned this pull request Jan 25, 2024

Ignore invalid Unicode in pkg-config descriptions (backport #9609) #9658

Merged

mergify bot added a commit that referenced this pull request Jan 25, 2024

Merge pull request #9658 from haskell/mergify/bp/3.10/pr-9609

9f9b5bd

Ignore invalid Unicode in pkg-config descriptions (backport #9609)

geekosaur mentioned this pull request Mar 8, 2024

cabal fails to use pkg-config for dependency resolution on linux #9790

Closed

crocodile-dentist mentioned this pull request Mar 15, 2024

Cannot build version 8.1.2 due to a missing libsodium library. IntersectMBO/cardano-node#5528

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore invalid Unicode in pkg-config descriptions #9609

Ignore invalid Unicode in pkg-config descriptions #9609

tomsmeding commented Jan 11, 2024 •

edited

Loading

alt-romes left a comment

alt-romes Jan 11, 2024

tomsmeding Jan 11, 2024

alt-romes Jan 11, 2024 •

edited

Loading

tomsmeding Jan 11, 2024

Mikolaj left a comment

mpickering Jan 11, 2024

tomsmeding Jan 11, 2024

mpickering Jan 11, 2024

tomsmeding Jan 11, 2024

Mikolaj Jan 11, 2024

Mikolaj commented Jan 11, 2024

michaelpj Jan 11, 2024

tomsmeding Jan 11, 2024

Mikolaj commented Jan 11, 2024

Mikolaj commented Jan 12, 2024

tomsmeding commented Jan 12, 2024 •

edited

Loading

Mikolaj commented Jan 15, 2024 •

edited

Loading

Mikolaj commented Jan 15, 2024

mergify bot commented Jan 15, 2024

mpickering Jan 15, 2024

gbaz Jan 23, 2024

Mikolaj commented Jan 24, 2024

Mikolaj commented Jan 25, 2024

mergify bot commented Jan 25, 2024 •

edited

Loading

tomsmeding commented Jan 25, 2024

	pkgList <- LBS.split 10 <$> getProgramInvocationLBS verbosity (programInvocation pkgConfig ["--list-all"])
	pkgList <- LBS.split (fromEnum '\n') <$> getProgramInvocationLBS verbosity (programInvocation pkgConfig ["--list-all"])

Ignore invalid Unicode in pkg-config descriptions #9609

Ignore invalid Unicode in pkg-config descriptions #9609

Conversation

tomsmeding commented Jan 11, 2024 • edited Loading

alt-romes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alt-romes Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikolaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikolaj commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikolaj commented Jan 11, 2024

Mikolaj commented Jan 12, 2024

tomsmeding commented Jan 12, 2024 • edited Loading

Mikolaj commented Jan 15, 2024 • edited Loading

Mikolaj commented Jan 15, 2024

mergify bot commented Jan 15, 2024

✅ Branch has been successfully rebased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikolaj commented Jan 24, 2024

Mikolaj commented Jan 25, 2024

mergify bot commented Jan 25, 2024 • edited Loading

✅ Backports have been created

tomsmeding commented Jan 25, 2024

tomsmeding commented Jan 11, 2024 •

edited

Loading

alt-romes Jan 11, 2024 •

edited

Loading

tomsmeding commented Jan 12, 2024 •

edited

Loading

Mikolaj commented Jan 15, 2024 •

edited

Loading

mergify bot commented Jan 25, 2024 •

edited

Loading