Ensure that FilePaths don't contain interior NULs #279

hasufell · 2023-03-25T07:36:15Z

Follows:

@bgamari @Bodigrim

System/Posix/ByteString/FilePath.hsc

Bodigrim · 2023-04-07T18:23:15Z

Overall looks good. @vdukhovni could you please take a look?

System/Posix/ByteString/FilePath.hsc

System/Posix/PosixPath/FilePath.hsc

vdukhovni

As mentioned in upthread comments, I think that just using useCStringLen is simpler and faster.

System/Posix/ByteString/FilePath.hsc

vdukhovni · 2023-04-20T21:36:18Z

System/Posix/ByteString/FilePath.hsc

+          , ioe_location = "checkForInteriorNuls"
+          , ioe_description = "POSIX filepaths must not contain internal NUL octets."
+          , ioe_errno = Nothing
+          , ioe_filename = Just (either (error . show) id


Under what conditions are we hiding a bottom (error . show) inside ioe_filename?
I don't think that examining the error should ever blow up...

We are not. This function is total due to https://hackage.haskell.org/package/base-4.18.0.0/docs/GHC-IO-Encoding-Failure.html#v:TransliterateCodingFailure

This is even tested: https://github.com/haskell/filepath/blob/98f8bba9eac8c7183143d290d319be7df76c258b/tests/abstract-filepath/EncodingSpec.hs#L102

We are not. This function is total due to https://hackage.haskell.org/package/base-4.18.0.0/docs/GHC-IO-Encoding-Failure.html#v:TransliterateCodingFailure

This is even tested: https://github.com/haskell/filepath/blob/98f8bba9eac8c7183143d290d319be7df76c258b/tests/abstract-filepath/EncodingSpec.hs#L102

Then I think it would be more clear to return Nothing for the ioe_filename if unexpectedly that function fails anyway, and otherwise return Just the decoded filename.

either (const Nothing) Just :: Either a b -> Maybe b

I disagree. Using Nothing seems to potentially mask a theoretical bug in the encoders or base library. Under the hood we use unsafePerformIO and certain low level IO primitives.

I want the code to crash, but I don't think it's worthwhile to move it into IO either.

Examining fields of a thrown exception should never throw another error, no matter how unlikely that might be.
Another alternative might be to simply render the filename octets as characters with no attempt to interpret as UTF8. Then the whole issue goes away. It is not uncommon for filenames to be in some 8bit charset that isn't UTF8.

That _toStr example is also wrong IMNSHO. When we have an octet string with no hope of knowing whether it is some encoded text string, or just 8-bit data (sans NUL octets), or when it is a text string, what encoding produced it, the best we can do is render exactly one character per octet, without any "decoding".

Note also that in that context Nothing is not an option, as it is with ioe_filename. It is not OK to ever booby-trap the IOError object fields with latent bottoms.

That _toStr example is also wrong IMNSHO. When we have an octet string with no hope of knowing whether it is some encoded text string, or just 8-bit data (sans NUL octets), or when it is a text string, what encoding produced it, the best we can do is render exactly one character per octet, without any "decoding".

There's nothing wrong about this, as these are user visible error messages, not proper filepaths.

I'm well aware of all the encoding intricacies of filepaths, since I've implemented the AFPP.

This is a tradeoff choice.

Another angle that I'm willing to discuss is whether ioe_filename should allow the caller to recover the actual filename or whether it's just descriptive.

In that context, converting every Word8 to a Char is very specific semantics the caller would have to know about. I'd go so far to say it's wrong on this level, since most other stuff in base uses getFileSystemEncoding. So we'd use decodeFS (which has correctness issues too, btw. and is in IO).

So, I don't think there is any right choice if we want the caller to be able to recover the filename.

If we just want nice printing, the current solution is ok.

It is not OK to ever booby-trap the IOError object fields with latent bottoms.

I agree and our tests could trivially catch this. As I pointed out, these functions are total and error is unreachable.

So, Bytestring modules do the infamous unpack:

unix/System/Posix/ByteString/FilePath.hsc

Line 133 in dc91c94

throwErrnoIfMinus1_ (loc <> " '" <> BC.unpack path1 <> "' to '" <> BC.unpack path2 <> "'")

I'm rather negative to this approach and would argue the fact that this exists is due to lack of decodeFS for bytestring (which isn't an issue in OsPath modules).

However, one could make the consistency argument.

Bodigrim · 2023-07-11T20:51:53Z

@vdukhovni @hs-viktor @hasufell is there any conclusion? We have to make a release soon, to accompany GHC 9.8.

hasufell · 2023-07-12T01:53:01Z

We haven't come to a conclusion wrt ioe_filename and I was actually planning to consult with GHC devs about this. Or even CLC. It's not clear what the API of this record is to the end user.

Based on that, we can more easily decide what to do.

vdukhovni · 2023-07-12T03:02:47Z

I don't feel very strongly about encoding choices, but I am quite adamant about error values not containing bottoms.
Pick the encoding strategy that will be least surprising, and never ever have any of the fields of IOError throw when forced.

When working with paths that are ByteStrings, if they make sense as UTF8, assume they're UTF8, otherwise raw?
Assuming the default system filename encoding (as suggested) is another. No encoding choice will be always right, we simply don't know. Part of the reason the IO operation failed, could well be a poor choice of encoding... :-(

hasufell · 2023-07-12T03:47:08Z

but I am quite adamant about error values not containing bottoms

Yes, as I explained above, the current code using TransliterateCodingFailure never throws.

Whatever we do, we have to document it in base, IMO: https://hackage.haskell.org/package/base-4.18.0.0/docs/GHC-IO-Exception.html#v:ioe_filename

It'll be utterly confusing to end users otherwise, looking for the properties of this record field.

System.Posix.ByteString.FilePath just unpacks, while System.Posix.PosixPath.FilePath tried to be smart and decode with UTF (without failing). This divergence was discussed here: * haskell#279 * haskell/core-libraries-committee#189

Bodigrim · 2023-07-23T10:47:23Z

We haven't come to a conclusion wrt ioe_filename and I was actually planning to consult with GHC devs about this. Or even CLC. It's not clear what the API of this record is to the end user.

Based on that, we can more easily decide what to do.

AFAIU this is no longer a blocker, right?

hasufell · 2023-07-23T10:57:02Z

Correct. I need to update the PR.

Follow: * https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10110 * haskell/core-libraries-committee#144

hasufell · 2023-07-23T14:43:02Z

Should be settled.

See: * haskell/core-libraries-committee#189 * haskell/unix#279 * haskell/unix#289

hasufell force-pushed the disallow-filepaths-with-nul branch from 15d5846 to 80cf9c3 Compare March 25, 2023 07:44

hasufell mentioned this pull request Mar 25, 2023

Reject FilePaths containing interior NULs haskell/core-libraries-committee#144

Closed

hasufell force-pushed the disallow-filepaths-with-nul branch from 80cf9c3 to 338dc25 Compare March 25, 2023 14:52

Bodigrim reviewed Apr 7, 2023

View reviewed changes

System/Posix/ByteString/FilePath.hsc Outdated Show resolved Hide resolved

Bodigrim requested a review from hs-viktor April 7, 2023 18:22

hasufell force-pushed the disallow-filepaths-with-nul branch from 338dc25 to fa57c3b Compare April 11, 2023 14:46

vdukhovni reviewed Apr 11, 2023

View reviewed changes

System/Posix/ByteString/FilePath.hsc Outdated Show resolved Hide resolved

vdukhovni reviewed Apr 11, 2023

View reviewed changes

System/Posix/PosixPath/FilePath.hsc Outdated Show resolved Hide resolved

vdukhovni suggested changes Apr 11, 2023

View reviewed changes

hasufell force-pushed the disallow-filepaths-with-nul branch from fa57c3b to 39eb676 Compare April 12, 2023 12:45

hasufell requested a review from vdukhovni April 12, 2023 12:45

vdukhovni suggested changes Apr 12, 2023

View reviewed changes

System/Posix/ByteString/FilePath.hsc Outdated Show resolved Hide resolved

hasufell force-pushed the disallow-filepaths-with-nul branch from 39eb676 to 145d17b Compare April 20, 2023 10:13

hasufell requested review from vdukhovni and Bodigrim April 20, 2023 10:13

Bodigrim approved these changes Apr 20, 2023

View reviewed changes

vdukhovni suggested changes Apr 20, 2023

View reviewed changes

hasufell mentioned this pull request Jul 13, 2023

[Question] purpose and encoding of ioe_filename haskell/core-libraries-committee#189

Closed

hasufell mentioned this pull request Jul 17, 2023

Make error strings consistent #288

Closed

Ensure that FilePaths don't contain interior NULs

2455442

Follow: * https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10110 * haskell/core-libraries-committee#144

hasufell force-pushed the disallow-filepaths-with-nul branch from 145d17b to 2455442 Compare July 23, 2023 14:42

hasufell requested a review from Bodigrim July 23, 2023 14:42

hasufell requested a review from vdukhovni July 23, 2023 14:42

vdukhovni approved these changes Jul 23, 2023

View reviewed changes

Bodigrim approved these changes Jul 23, 2023

View reviewed changes

Bodigrim merged commit 5ff1926 into haskell:master Jul 23, 2023

tbagrel1 pushed a commit to tweag/ghc that referenced this pull request Aug 2, 2023

Improve documentation around IOException and ioe_filename

42aa7fb

See: * haskell/core-libraries-committee#189 * haskell/unix#279 * haskell/unix#289

Ensure that FilePaths don't contain interior NULs #279

Ensure that FilePaths don't contain interior NULs #279

Uh oh!

Conversation

hasufell commented Mar 25, 2023

Uh oh!

Uh oh!

Bodigrim commented Apr 7, 2023

Uh oh!

Uh oh!

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bodigrim commented Jul 11, 2023

Uh oh!

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

Uh oh!

Uh oh!

vdukhovni commented Jul 12, 2023

Uh oh!

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

Uh oh!

Uh oh!

Bodigrim commented Jul 23, 2023

Uh oh!

hasufell commented Jul 23, 2023

Uh oh!

hasufell commented Jul 23, 2023

Uh oh!

Uh oh!

hasufell Apr 23, 2023 •

edited

Loading

hasufell commented Jul 12, 2023 •

edited

Loading

hasufell commented Jul 12, 2023 •

edited

Loading