Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[George Valkov]

> On 2023-02-20, at 11:14 PM, Pádraig Brady  wrote:
> 
> On 20/02/2023 19:35, George Valkov wrote:
>>> On 2023-02-20, at 7:49 PM, Pádraig Brady  wrote:
>>> 
>>> On 20/02/2023 15:02, George Valkov wrote:
 Hi Paul, the following tests fail after your patch:
 FAIL: tests/split/filter.sh
 FAIL: tests/split/b-chunk.sh
 FAIL: tests/split/l-chunk.sh
>>> 
>>> These look unrelated and may be due to some other change in your 
>>> environment.
>>> Specifically lseek() is failing on /dev/null and returning:
>>> split: /dev/null: cannot determine file size
>> I deleted the lines that were introduced by the patch in unistd.in.h, then 
>> make clean; make -j 16 and ran all tests: back to 5 failed. Then I restored 
>> the deleted lines, rebuild and I got 9 failed tests again.
> 
> Oh right sorry.
> I think I see what's happening.
> Since https://github.com/coreutils/gnulib/commit/4db8db34
> gnulib has been unconditionally replacing lseek() on macos.
> Now with SEEK_DATA undefined that replaced lseek()
> will run the code intended for BeOS.
> So either the lseek.m4 or lseek.c needs adjusting accordingly.

As I reported in one of my previous comments, there is no need for that hack in 
lseek.c. Either use the original lseek from macOS and make sure it doesn't 
return cached data, or disable sparse support until it gets fixed by Apple. The 
more people report it to them the higher the chance for them to take any action.
Let me know when you fix it.

Good luck!


Georgi Valkov
httpstorm.com
nano RTOS

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 20/02/2023 19:35, George Valkov wrote:



On 2023-02-20, at 7:49 PM, Pádraig Brady  wrote:

On 20/02/2023 15:02, George Valkov wrote:

Hi Paul, the following tests fail after your patch:
FAIL: tests/split/filter.sh
FAIL: tests/split/b-chunk.sh
FAIL: tests/split/l-chunk.sh


These look unrelated and may be due to some other change in your environment.
Specifically lseek() is failing on /dev/null and returning:
split: /dev/null: cannot determine file size


I deleted the lines that were introduced by the patch in unistd.in.h, then make 
clean; make -j 16 and ran all tests: back to 5 failed. Then I restored the 
deleted lines, rebuild and I got 9 failed tests again.


Oh right sorry.
I think I see what's happening.
Since https://github.com/coreutils/gnulib/commit/4db8db34
gnulib has been unconditionally replacing lseek() on macos.
Now with SEEK_DATA undefined that replaced lseek()
will run the code intended for BeOS.
So either the lseek.m4 or lseek.c needs adjusting accordingly.

cheers,
Pádraig

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[George Valkov]

> On 2023-02-20, at 7:49 PM, Pádraig Brady  wrote:
> 
> On 20/02/2023 15:02, George Valkov wrote:
>> Hi Paul, the following tests fail after your patch:
>> FAIL: tests/split/filter.sh
>> FAIL: tests/split/b-chunk.sh
>> FAIL: tests/split/l-chunk.sh
> 
> These look unrelated and may be due to some other change in your environment.
> Specifically lseek() is failing on /dev/null and returning:
> split: /dev/null: cannot determine file size

I deleted the lines that were introduced by the patch in unistd.in.h, then make 
clean; make -j 16 and ran all tests: back to 5 failed. Then I restored the 
deleted lines, rebuild and I got 9 failed tests again.


>> FAIL: tests/cp/sparse-perf.sh
> 
> As expected.
> I've a fix for this locally
> which leverages another patch to determine
> dynamically if we support SEEK_HOLE.

All right! Cheers mate!


Georgi Valkov
httpstorm.com
nano RTOS

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[Pádraig Brady]

On 20/02/2023 15:02, George Valkov wrote:

Hi Paul, the following tests fail after your patch:
FAIL: tests/split/filter.sh
FAIL: tests/split/b-chunk.sh
FAIL: tests/split/l-chunk.sh


These look unrelated and may be due to some other change in your environment.
Specifically lseek() is failing on /dev/null and returning:
split: /dev/null: cannot determine file size


FAIL: tests/cp/sparse-perf.sh


As expected.
I've a fix for this locally
which leverages another patch to determine
dynamically if we support SEEK_HOLE.

cheers,
Pádraig

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[George Valkov]

Hi Paul, the following tests fail after your patch:
FAIL: tests/split/filter.sh
FAIL: tests/split/b-chunk.sh
FAIL: tests/split/l-chunk.sh
FAIL: tests/cp/sparse-perf.sh

Before patch
https://httpstorm.com/share/.openwrt/test/2023-02-06_coreutils-9.1/test-05-95f4ee0577dd836de523f46999777fbbbe9d2772-ori.txt

After patch
https://httpstorm.com/share/.openwrt/test/2023-02-06_coreutils-9.1/test-05-95f4ee0577dd836de523f46999777fbbbe9d2772-patch.txt


Georgi Valkov
httpstorm.com
nano RTOS

Re: bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[George Valkov]

> On 2023-02-19, at 8:08 AM, Paul Eggert  wrote:
> 
> George, given what you've written I suppose we should give up the idea of 
> copying sparse files efficiently on macOS (and on FreeBSD 13.0-RELEASE, as it 
> has a similar bug with SEEK_HOLE and SEEK_DATA), in cases where fclonefileat 
> does not work.

I agree, Paul. I just ran some benchmarks: the time it takes to call msync 
unconditionally hurts the performance: 502 us if the memory view is already 
synced seems fine, however 28177-36585 us whenever a sync is required is more 
than the time it takes to performing a full-copy 7042-21528 us.


> Please try the attached Gnulib patch; it uses your idea of disabling 
> SEEK_HOLE and SEEK_DATA on macOS, except on steroids because it disables 
> these two macros everywhere, for all Gnulib-using applications, and it also 
> disables them on FreeBSD < 14. For coreutils you need only this patch's 
> change to lib/unistd.in.h. If this works for you I suppose we can install it 
> into Gnulib and propagate that into coreutils etc.

What is the correct way to apply your patch? I tried
git apply 0001-lseek-avoid-SEEK_HOLE-bugs-in-FreeBSD-macOS.patch 
error: patch failed: ChangeLog:1
error: ChangeLog: patch does not apply
error: doc/posix-functions/lseek.texi: No such file or directory
error: lib/unistd.in.h: wrong type

Then I made the changes to lib/unistd.in.h manually, and ran the tests:
* before: corrupted
* after: fixed

Regarding FreeBSD, I have a FreeBSD 13.1-RELEASE VM running on Hyper-V, so I 
compiled and ran my samples: m.c, d.c: the copy was correct, however d.c 
reports that there are no segments to skip, which means that the file created 
by m.c using mmap is not sparse even though it should contain one blank segment 
and be sparse. Next I installed a fresh copy of FreeBSD 13.0-RELEASE and 
observed the same behaviour. Both installations use ZFS and the tests were ran 
as root.
If you know what conditions are required to reproduce the issue on FreeBSD, let 
me know and I will test it for you. Otherwise I am not sure if we need to make 
any changes to FreeBSD, since I did not observe anything wrong there.
Is it ok if I continue testing on FreeBSD 13.1, because the 13.0 I just 
installed lacks any configuration and is quite uncomfortable to use. It also 
rejects my password over SSH, so I’m forced to use the console. I can test 
pretty much anything that runs on Hyper-V.


> Also, you reported the bug against macOS 12.6. Can you check whether the same 
> bug occurs on macOS 13? If it's still there I suppose the attached patch will 
> need to be updated since it guesses the bug is fixed in macOS 13.

Yes it does. I just ran my samples under macOS Internet recovery, which reports 
as Ventura with kernel compiled on the 30th of January 2023. See the attached 
log:
uname -a
Darwin gMac.lan 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:42:11 PST 
2023; root:xnu-8792.81.3~2/RELEASE_X86_64 x86_64

./m 
src 3  dst 4
1000 skip
total bytes copied 1a45640 / 1a46640 1a46640  100535 us

/gfc cc1-mmap 
16d835378ab973a114082a585cc76958bdbccec0  cc1-mmap

./d
src 3  dst 4
c a46640  p 1a46640  h 1a46640  d 100
total bytes copied a46640 / 1a46640  7117 us

./gfc cc1-sparse 
75e6f6cb303cb5d3909c6d7830c417fc6ed658c3  cc1-sparse

./n 
dst 3  MS_ASYNC 1  MS_INVALIDATE 2  MS_SYNC 16
size bytes 1a46640  msync ok  43770 us

./d 
src 3  dst 4
c 1000  p 1000  h 1000  d 0
c 1a44640  p 1a46640  h 1a46640  d 2000
total bytes copied 1a45640 / 1a46640  41322 us

./gfc cc1-sparse 
16d835378ab973a114082a585cc76958bdbccec0  cc1-sparse

./m 
src 3  dst 4
1000 skip
total bytes copied 1a45640 / 1a46640 1a46640  59198 us

./d 
src 3  dst 4
c a46640  p 1a46640  h 1a46640  d 100
total bytes copied a46640 / 1a46640  12144 us

./gfc cc1-sparse 
75e6f6cb303cb5d3909c6d7830c417fc6ed658c3  cc1-sparse

./n 
dst 3  MS_ASYNC 1  MS_INVALIDATE 2  MS_SYNC 16
size bytes 1a46640  msync ok  27462 us

./d 
src 3  dst 4
c 1000  p 1000  h 1000  d 0
c 1a44640  p 1a46640  h 1a46640  d 2000
total bytes copied 1a45640 / 1a46640  32125 us

./gfc cc1-sparse 
16d835378ab973a114082a585cc76958bdbccec0  cc1-sparse





> I'll cc this email to bug-gnulib to give them a heads-up. (Gnulib readers can 
> see  for context.)

Thanks, I’ll check it.


> Really, Apple should fix this serious data corruption bug in APFS and macOS.
> <0001-lseek-avoid-SEEK_HOLE-bugs-in-FreeBSD-macOS.patch>

I agree, I opened a new ticket with Apple on 2023-02-16. Both via e-mail, and 
https://security.apple.com/reports/OE1928608366012

This was at the same time I reported about mmap and msync here. I got this 
replay on the following day:
> Thanks for the additional information. We are reviewing it and will follow up 
> if we have any questions or need additional details.

Feel free to write them at product-secur...@apple.com and include this line
OE0928602070811 - please include this ID in replies to this thread.

Introduce your self as part of G