Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disk_*_space uses statvfs on macOS and it returns inaccurate results for some filesystems #8048

Closed
risner opened this issue Feb 6, 2022 · 5 comments

Comments

@risner
Copy link
Contributor

risner commented Feb 6, 2022

Description

The following code:

<?php print disk_total_space("/nfs1") . "\n";?>

Resulted in this output:

198919206912

But I expected this output instead:

60518056220

The code works correctly for local filesystems, and fails on NFS.
If I remember correctly the posix standard is not required to provide correct information. Should php never be using the statvfs call?

Data points for NFS:
php total 198919206912
correct total 30985244784640
df total 30259028110
vfs total 194257038
fs f_bsize 512 vfs 1048576
fs f_iosize 1048576 vfs f_frsize 512
fs f_blocks 60518056220 vfs 388514076
fs f_bfree 20573829227 vfs 3393960043
fs f_bavail 20573829227 vfs 3393960043

Data points for local:
php total 245107195904 vfs 245107195904
correct total 245107195904
df total 3496966385312
vfs total 3496966385312
fs f_bsize 4096 vfs 1048576
fs f_iosize 1048576 vfs f_frsize 4096
fs f_blocks 59840624 vfs 59840624
fs f_bfree 5153807 vfs 5153807
fs f_bavail 5153807 vfs 5153807

PHP Version

PHP 7.4

Operating System

macOS 12.1

@risner
Copy link
Contributor Author

risner commented Feb 6, 2022

Additionally, on FreeBSD there is a standards note:
STANDARDS
The statvfs() and fstatvfs() functions conform to IEEE Std 1003.1-2001
(“POSIX.1”). As standardized, portable applications cannot depend on
these functions returning any valid information at all.
This
implementation attempts to provide as much useful information as is
provided by the underlying file system, subject to the limitations of the
specified data types.

@risner
Copy link
Contributor Author

risner commented Feb 7, 2022

Looking at macOS's Libc-1353.11.2, it does a conversion from statfs to statvfs in libc.
It is an integer overflow issue.
size x64 = 8; size x32 = 4
x64 = 60518056220; x32 = 388514076

For reliable results, on macOS the source should be statfs(2) instead of statvfs(3). I suspect this is a good practice to do whenever statfs is also available. Linux my be the exception, I found a number of statements regarding Linux doing statvfs correctly while researching this issue.

Code snip of the function:
/* Internal common conversion function */
static void
cvt_statfs_to_statvfs(struct statfs *from, struct statvfs *to)
{
to->f_bsize = from->f_iosize;
to->f_frsize = from->f_bsize;
to->f_blocks = (fsblkcnt_t)from->f_blocks;
to->f_bfree = (fsblkcnt_t)from->f_bfree;

@devnexen
Copy link
Member

devnexen commented Feb 7, 2022

are you able to propose a PR eventually ?

@risner
Copy link
Contributor Author

risner commented Feb 7, 2022

#8056

@risner risner changed the title disk_*_space uses statvfs on macOS and it returns inaccruate results for some filesystems disk_*_space uses statvfs on macOS and it returns inaccurate results for some filesystems Feb 7, 2022
@cmb69 cmb69 linked a pull request Mar 4, 2022 that will close this issue
cmb69 added a commit that referenced this issue Mar 4, 2022
* PHP-8.0:
  Fix GH-8048: disk_*_space wrong for some filesystems on macOS
@cmb69 cmb69 closed this as completed in 57ef16b Mar 4, 2022
cmb69 added a commit that referenced this issue Mar 4, 2022
* PHP-8.1:
  Fix GH-8048: disk_*_space wrong for some filesystems on macOS
@cmb69
Copy link
Member

cmb69 commented Mar 4, 2022

Thanks for the PR, and sorry it tooks so long to merge! The fix will only be available as of PHP 8.0.18 and 8.1.5, respectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@cmb69 @devnexen @risner and others