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

Unexpected stat(v)fs struct size on i686 #71

Closed
tatokis opened this issue Sep 8, 2024 · 19 comments · Fixed by #73
Closed

Unexpected stat(v)fs struct size on i686 #71

tatokis opened this issue Sep 8, 2024 · 19 comments · Fixed by #73

Comments

@tatokis
Copy link
Contributor

tatokis commented Sep 8, 2024

The following two tests fail when running on a 32 bit x86 system.

==> Starting check()...
/usr/bin/ruby -I/usr/lib/ruby/gems/3.2.0/gems/rspec-support-3.13.1/lib:/usr/lib/ruby/gems/3.2.0/gems/rspec-core-3.13.0/lib /usr/lib/ruby/gems/3.2.0/gems/rspec-core-3.13.0/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb
Run options: exclude {:dragonfly=>true, :windows=>true}
............................................................................................FF...

Failures:

  1) Sys::Filesystem FFI statfs struct is expected size
     Failure/Error: expect(Sys::Filesystem::Structs::Statfs.size).to eq(dummy.check_sizeof('struct statfs', header))

       expected: 64
            got: 84

       (compared using ==)
     # ./spec/sys_filesystem_unix_spec.rb:505:in `block (3 levels) in <top (required)>'

  2) Sys::Filesystem FFI statvfs struct is expected size
     Failure/Error: expect(Sys::Filesystem::Structs::Statvfs.size).to eq(dummy.check_sizeof('struct statvfs', 'sys/statvfs.h'))

       expected: 72
            got: 92

       (compared using ==)
     # ./spec/sys_filesystem_unix_spec.rb:509:in `block (3 levels) in <top (required)>'

Finished in 0.38878 seconds (files took 0.11209 seconds to load)
97 examples, 2 failures
@djberg96
Copy link
Owner

djberg96 commented Sep 9, 2024

Hm, I've tried dealing with this multiple times. There was #65 and #59.

Please show me the output of uname -a and cat /etc/*release. Also, please show me the output of:

  • RbConfig::CONFIG['DEFS']
  • RbConfig::CONFIG['host_os']
  • RbConfig::CONFIG['arch']

@tatokis
Copy link
Contributor Author

tatokis commented Sep 9, 2024

[builduser@arch-nspawn-1352508 ~]$ uname -a
Linux arch-nspawn-1352508 6.10.1 #1 SMP PREEMPT_DYNAMIC Fri Jul 26 22:44:32 EEST 2024 i686 GNU/Linux
[builduser@arch-nspawn-1352508 ~]$ ruby
p RbConfig::CONFIG['DEFS']
p RbConfig::CONFIG['host_os']   
p RbConfig::CONFIG['arch']

"-D_FILE_OFFSET_BITS=64"
"linux"
"i686-linux"

The following returns nothing inside the build container (probably because the appropriate packages weren't installed), so the following output is from a bare metal machine.

$ cat /etc/*release

DISTRIB_ID="Arch"
DISTRIB_RELEASE="rolling"
DISTRIB_DESCRIPTION="Arch Linux"
NAME="Arch Linux 32"
PRETTY_NAME="Arch Linux 32"
ID=arch32
ID_LIKE=arch
BUILD_ID=rolling
ANSI_COLOR="38;2;23;147;209"
HOME_URL="https://www.archlinux32.org/"
DOCUMENTATION_URL="https://wiki.archlinux.org/"
SUPPORT_URL="https://bbs.archlinux32.org/"
BUG_REPORT_URL="https://bugs.archlinux32.org/"
PRIVACY_POLICY_URL="https://terms.archlinux.org/docs/privacy-policy/"
LOGO=archlinux-logo

@djberg96
Copy link
Owner

djberg96 commented Sep 9, 2024

I guess FILE_OFFSET_BITS=64 is no guarantee. Try changing https://github.com/djberg96/sys-filesystem/blob/main/lib/sys/unix/sys/filesystem/functions.rb#L19 from || to && and let me know how that works.

Edit: actually, I think that's not the correct solution.

@djberg96
Copy link
Owner

djberg96 commented Sep 9, 2024

Could just be that the spec needs to be updated. I feel like this is an arch-specific issue, too.

@tatokis
Copy link
Contributor Author

tatokis commented Sep 9, 2024

That statement seems correct with ||.

It definitely evaluates to true.

[builduser@arch-nspawn-2259220 ~]$ ruby
p RbConfig::CONFIG['host_os'] =~ /linux/i && (RbConfig::CONFIG['arch'] =~ /64/ || RbConfig::CONFIG['DEFS'] =~ /64/)

20

which means that linux64 should be true, thus we should expect the larger structure.

[builduser@arch-nspawn-2259220 ~]$ cat test.c 
#include <sys/vfs.h>
#include <stdio.h>

int main()
{
    printf("struct statfs size %zu\n", sizeof(struct statfs));
    return 0;
}
[builduser@arch-nspawn-2259220 ~]$ gcc test.c
[builduser@arch-nspawn-2259220 ~]$ file a.out
a.out: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux.so.2, BuildID[sha1]=08dc755e3ebe62a2a16a6400507580d57462f579, for GNU/Linux 4.4.0, not stripped
[builduser@arch-nspawn-2259220 ~]$ ./a.out
struct statfs size 64
[builduser@arch-nspawn-2259220 ~]$ gcc test.c -D_FILE_OFFSET_BITS=64
[builduser@arch-nspawn-2259220 ~]$ ./a.out
struct statfs size 84

which makes sense, as the test itself says got: 84.

Running check_sizeof('struct statfs', 'sys/statfs.h'), however, returns 64, so it looks like mkmf-lite needs to pass -D_FILE_OFFSET_BITS=64 to the compiler somehow. Tried exporting both CC="gcc -D_FILE_OFFSET_BITS=64" and CFLAGS="-D_FILE_OFFSET_BITS=64" but it looks like it ignores them.

@djberg96
Copy link
Owner

djberg96 commented Sep 9, 2024

Does arch have separate structs, e.g. statfs and statfs64?

@tatokis
Copy link
Contributor Author

tatokis commented Sep 10, 2024

Arch uses glibc which only provides struct statfs64 and co with -D_LARGEFILE64_SOURCE=1. That said, feature_test_macros(7) on my system states:

New programs should not employ this macro; instead _FILE_OFFSET_BITS=64 should be employed.

Without that define you get

test.c:6:47: error: invalid application of ‘sizeof’ to incomplete type ‘struct statfs64’
    6 |     printf("struct statfs size %zu\n", sizeof(struct statfs64));
      |                                               ^~~~~~

I also found out that musl provides 64 bit functions only. I don't know if anyone has tried this gem on a musl system but I suspect there it'll expect the 32 bit variants and fail (since it won't find the 64 in defs).

@djberg96
Copy link
Owner

mkmf-lite only uses whatever's inside RbConfig::CONFIG rather than ENV. Trying to think of a good solution here.

@tatokis
Copy link
Contributor Author

tatokis commented Sep 11, 2024

It doesn't seem to be using RbConfig::CONFIG['DEFS'] in this case though, either, which does have "-D_FILE_OFFSET_BITS=64" inside of it. If it was using that, then it wouldn't be returning 64 instead of 84 for statfs.

@djberg96
Copy link
Owner

Please try with mkmf-lite 0.7.0 and let me know how it goes.

@tatokis
Copy link
Contributor Author

tatokis commented Sep 16, 2024

The statfs one passes now, but the statvfs still fails. It looks like the issue is different now (possibly incorrect spec file).

.............................................................................................F...

Failures:

  1) Sys::Filesystem FFI statvfs struct is expected size
     Failure/Error: expect(Sys::Filesystem::Structs::Statvfs.size).to eq(dummy.check_sizeof('struct statvfs', 'sys/statvfs.h'))
     
       expected: 96
            got: 92
     
       (compared using ==)
     # ./spec/sys_filesystem_unix_spec.rb:509:in `block (3 levels) in <top (required)>'

Finished in 0.38867 seconds (files took 0.10147 seconds to load)
97 examples, 1 failure

Failed examples:

rspec ./spec/sys_filesystem_unix_spec.rb:508 # Sys::Filesystem FFI statvfs struct is expected size

With a similar test program as above: sizeof(struct statvfs) is 72 without -D_FILE_OFFSET_BITS=64 and 96 with -D_FILE_OFFSET_BITS=64, so mkmf is in fact reporting the correct size now.

@djberg96
Copy link
Owner

I think now we'll need to compare the actual statvfs struct on your system versus what I've got defined in structs.rb.

@tatokis
Copy link
Contributor Author

tatokis commented Sep 16, 2024

Snippet from my bits/statvfs.h

#include <bits/types.h>  /* For __fsblkcnt_t and __fsfilcnt_t.  */

#if (__WORDSIZE == 32 \
     && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
#define _STATVFSBUF_F_UNUSED
#endif

struct statvfs
  {
 unsigned long int f_bsize;
    unsigned long int f_frsize;
#ifndef __USE_FILE_OFFSET64
    __fsblkcnt_t f_blocks;
    __fsblkcnt_t f_bfree;
    __fsblkcnt_t f_bavail;
    __fsfilcnt_t f_files;
    __fsfilcnt_t f_ffree;
    __fsfilcnt_t f_favail;
#else
    __fsblkcnt64_t f_blocks;
    __fsblkcnt64_t f_bfree;
    __fsblkcnt64_t f_bavail;
    __fsfilcnt64_t f_files;
    __fsfilcnt64_t f_ffree;
    __fsfilcnt64_t f_favail;
#endif
    unsigned long int f_fsid;
#ifdef _STATVFSBUF_F_UNUSED
    int __f_unused;
#endif
    unsigned long int f_flag;
    unsigned long int f_namemax;
    unsigned int f_type;
    int __f_spare[5];
  };

If I am reading it correctly, it looks like it's because int __f_unused; is missing in the 64 bit struct [0] but exists on my system due to _STATVFSBUF_F_UNUSED being defined at the beginning. It'd make sense since sizeof(int) == 4 on this system, and the actual value is 4 bytes more than the expected.

[0]

layout(
:f_bsize, :ulong,
:f_frsize, :ulong,
:f_blocks, :uint64,
:f_bfree, :uint64,
:f_bavail, :uint64,
:f_files, :uint64,
:f_ffree, :uint64,
:f_favail, :uint64,
:f_fsid, :ulong,
:f_flag, :ulong,
:f_namemax, :ulong,
:f_spare, [:int, 6]
)

@djberg96
Copy link
Owner

I think this will require a PR. Since it's a one-time check on load it shouldn't be a huge deal. Feel like putting one together? Its probably easier for you to create and test than me since you have the system.

@tatokis
Copy link
Contributor Author

tatokis commented Sep 21, 2024

I'm unsure how this would be implemented. This extra field would need to be added to the linux64 struct.

Since I'm not a ruby developer, the only way I can think of doing this is by making a linux32_64 struct that has this extra field inside of it, and picking that instead of the existing linux64 one. Is this what you have in mind? Or is there some other mechanism I can use to modify the struct in a more manageable way?

@djberg96
Copy link
Owner

Hm, am I missing the f_type? I'll have to check the other distros, but that will take some time as my HDD crashed and I have to rebuild a bunch of VM's. In the meantime, does something like this work?

diff --git a/lib/sys/unix/sys/filesystem/structs.rb b/lib/sys/unix/sys/filesystem/structs.rb
index 0b7243f..e884cbe 100644
--- a/lib/sys/unix/sys/filesystem/structs.rb
+++ b/lib/sys/unix/sys/filesystem/structs.rb
@@ -226,7 +226,7 @@ module Sys
             :f_spare, [:int, 6]
           )
         else
-          layout(
+          layout_array = [
             :f_bsize, :ulong,
             :f_frsize, :ulong,
             :f_blocks, :uint64,
@@ -239,7 +239,14 @@ module Sys
             :f_flag, :ulong,
             :f_namemax, :ulong,
             :f_spare, [:int, 6]
-          )
+          ]
+
+          require 'mkmf-lite'
+          if have_struct_member('struct statvfs', 'f_type', 'sys/statvfs.h')
+            layout_array.insert(-3, :f_type, :int)
+          end
+
+          layout(*layout_array)
         end
       end

@djberg96
Copy link
Owner

I don't think f_type normally lives in statvfs, only statfs, but I did find this obscure post: https://www.openwall.com/lists/musl/2023/06/23/1. So maybe certain distros have it now?

tatokis added a commit to tatokis/sys-filesystem that referenced this issue Sep 27, 2024
@tatokis
Copy link
Contributor Author

tatokis commented Sep 27, 2024

The problem is not with f_type.

In my system's struct, f_spare is [5] + f_type, while in the ruby definition it's simply f_spare, [:int, 6] , which covers the missing f_type. The length of f_spare needs to be reduced by one if f_type is added. I have opened a PR.

The problem is that on the 64 bit struct under 32 bit systems, there's no int __f_unused; between f_fsid and f_flag.

Based on the diff above, I was initially going to do if have_struct_member('struct statvfs', '__f_unused', 'sys/statvfs.h'), however it looks like musl doesn't have that field at all. Instead, they do https://github.com/bminor/musl/blob/dd1e63c3638d5f9afb857fccf6ce1415ca5f1b8b/include/sys/statvfs.h#L20

@djberg96
Copy link
Owner

Your fixes have been merged and included in release 1.5.1 which I've just pushed out.

Many thanks for your help and contributions!

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

Successfully merging a pull request may close this issue.

2 participants