search

haskell / unix

POSIX functionality
https://hackage.haskell.org/package/unix
Other
107 stars 92 forks source link

Fix `withFilePath` #294

Closed hasufell closed 1 year ago

hasufell commented 1 year ago

https://github.com/haskell/cabal/pull/9241

@Bodigrim we'll need another release. This bug was introduced by 2.8.2.0, so maybe we can mark it as deprecated on hackage?

hasufell commented 1 year ago

@bgamari

vdukhovni commented 1 year ago

Sorry about that. I think the fault was mostly mine. Mind you the docs of useAsCStringLen could be more clear. They don't mention lack of NUL-termination, one has to carefully contrast with useAsCString to notice that only the latter mentions NUL-termination, and so implicitly perhaps the former does not NUL-terminate. I guess that choice is natural...

hasufell commented 1 year ago

Well, I was well aware of this intricacy, because I worked long enough on the ShortByteString code. So I'm not sure how that slipped through my radar either.

Bodigrim commented 1 year ago

Thanks for a quick turnaround!

I did not follow the discussion closely, is it possible to add a regression test?

In the meantime I've revised unix-2.8.2.0 with base < 0.

hasufell commented 1 year ago

Yes, we could generate random filepaths, run withFilePath and just print the CString.

I don't know if I will have time to do that today.

Bodigrim commented 1 year ago

A simple unit test will do IMHO, just to prevent someone in future to refactor back to useAsCStringLen.

hasufell commented 1 year ago

A simple unit test will do IMHO, just to prevent someone in future to refactor back to useAsCStringLen.

I don't think so. This is a memory bug and doesn't always trigger.

bgamari commented 1 year ago

Thanks for the heads up! Given the severity of the issue and the fact that it made it into a release, it would probably be a good idea to open a ticket and point to it from the changelog. I have done for former in #295; @hasufell, perhaps you can do the latter?

erikd commented 1 year ago

I just got hit by this. :tada:

Ran cabal update and now the bad version has been marked "unbuildable".

Bodigrim commented 1 year ago

@hasufell could you please rebase?

hasufell commented 1 year ago

rebased

hasufell commented 1 year ago

CI all green. Changelog updated. Version bumped to 2.8.2.1