alsa-project / alsa-lib

The Advanced Linux Sound Architecture (ALSA) - library
GNU Lesser General Public License v2.1
366 stars 177 forks source link

conf: Use LFS calls when reading config files #223

Closed dos1 closed 2 years ago

dos1 commented 2 years ago

Although at first glance it doesn't seem useful to support config files larger than 2GB, LFS also influences inode size. Without this, 32-bit libasound may be unable to read config files on filesystems with 64-bit inodes, such as Btrfs or NFS.

Signed-off-by: Sebastian Krzyszkowiak dos@dosowisko.net

dos1 commented 2 years ago

BTW. UCM code may need a similar treatment, but I didn't have a way to test it handy so I left it out.

bertogg commented 2 years ago

This problem should also be fixable by building alsa with -D_FILE_OFFSET_BITS=64.

evelikov commented 2 years ago

Agreed - all the other projects I've looked at are using -D_FILE_OFFSET_BITS=64 instead of changing the API.

bertogg commented 2 years ago

The potential problem is overflows, i.e. the code using the wrong data types (say int instead of off_t) or otherwise making assumptions about the range of the returned values. I think it's unlikely in this case but it's still worth pointing out and having a quick look.

evelikov commented 2 years ago

Note that for autoconf projects AC_SYS_LARGEFILE is usually used, instead of hard-coding the define stanza

dos1 commented 2 years ago

I think it's unlikely in this case but it's still worth pointing out and having a quick look.

off_t is being used in public function signatures. Building libasound with _FILE_OFFSET_BITS=64 via AC_SYS_LARGEFILE is what I tried initially, but it ends up breaking the ABI and causing crashes.

bertogg commented 2 years ago

off_t is being used in public function signatures.

Hmmm ok, I only see off_t being used in a couple of internal functions but if you say you found crashes there has to be something that I'm overlooking.

perexg commented 2 years ago

Thanks. I applied your change, tried to follow this change in the whole alsa-lib tree and I added '--with-lfs' option to configure for the cases, where the 64-bit LFS calls are not available. See above commits.

bertogg commented 2 years ago

Hello, I don't understand the purpose of --with-lfs, also if you disable that option ALSA won't build, neither in 32bit nor in 64bit.

  CC       parser.lo
In file included from ucm_local.h:39,
                 from parser.c:33:
../../include/local.h:82:18: error: redefinition of 'struct dirent'
   82 | #define dirent64 dirent
      |                  ^~~~~~
In file included from /usr/include/dirent.h:61,
                 from parser.c:36:
/usr/include/i386-linux-gnu/bits/dirent.h:22:8: note: originally defined here
   22 | struct dirent
      |        ^~~~~~
make[2]: *** [Makefile:393: parser.lo] Error 1
perexg commented 2 years ago

The option is available to forcefully skip the test. This option is not usable for glibc (_GNU_SOURCE=1, thus _LARGEFILE64_SOURCE=1 -> 64bit prototypes).

The assumption is that some libc libraries may not have 64-bit prototypes, thus it's fallback to the old behaviour. If this simple case (redefine) will cause issues, we need to use another structure and function names like (snd_scandir64) for the defines. It will work with glibc, too.

bertogg commented 2 years ago

My understanding is that if a library lacks the 64-bit prototypes then the configure script detects that automatically without the user having to do anything (hence the AC_TRY_LINK bit with have_lfs=no in case of failure).

So --without-lfs would only make sense if the library does have the 64-bit prototypes but you still want to disable them. I'm not sure if there's an actual use case for that but if you still want to have that option then it should work on glibc, shouldn't it?

perexg commented 2 years ago

As I wrote, I expected to refine this option when required. Ok, I removed this option in a19ce72310ee6687ea3312528f3182e7cf516957. It's easier fix and the use case for this option is not known.

bertogg commented 2 years ago

That sounds perfect, thanks !