libtraceevent: Have 32 bit compile with -D_FILE_OFFSET_BITS=64

Commit Message

Steven Rostedt Dec. 19, 2022, 10:34 p.m. UTC

From: "Steven Rostedt (Google)" <[e-mail]>

As stat() can overflow if a 32 bit user space is running on a 64 bit
kernel and the inode is more than 32 bits, compile with
_FILE_OFFSET_BITS=64 that mitigates this problem on 32 bit user space.

Do this by running the cross compiler cpp and checking what __LONG_SIZE__
is. If it returns 4, then add the define.

Reported-by: Mike Frysinger <[e-mail]>
Signed-off-by: Steven Rostedt (Google) <[e-mail]>
---
 Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Mike Frysinger Dec. 20, 2022, 7:36 p.m. UTC | #1

On Mon, Dec 19, 2022 at 5:34 PM Steven Rostedt wrote:
> +# Make sure 32 bit on 64 bit doesn't have stat() bugs.

i don't know what you're trying to say with "32 bit on 64 bit".  you
mean a 32 bit binary (which ABI?) with a 64 bit kernel ?  or something
else ?

the problem is entirely with using 32 bit filesystem interfaces.
there are no other runtime requirements, either in the userland or
kernel land.  filesystems moved to 64 bit fields in a number of places
long ago regardless of the userland or kernel ABIs.

saying "32 bit" or "64 bit" is imprecise.  is x86_64's x32 & aarch64's
ilp32 ABI "32 bit" ?  or is it "64 bit" ?  what about Window's x64
(LLP64) ?
https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
https://unix.org/version2/whatsnew/lp64_wp.html

usually it's better to specify ABI names and/or specific types so
people know what you're trying to cater to.

> +ifeq ($(shell echo '' | $(CPP) -dM - | grep __SIZEOF_LONG__ | cut -d' ' -f3),4)
> +       CFLAGS += -D_FILE_OFFSET_BITS=64
> +endif

why do you need to sniff the size of long ?  -D_FILE_OFFSET_BITS=64
should work fine regardless of the target ABI.

especially because you're assuming sizeof(long) == sizeof(off_t) which
is not guaranteed anywhere.  on newer ABIs, like x32, sizeof(off_t) is
always 64-bit while sizeof(long) is 32-bit.  i don't know if there's a
sizeof(long) == 8 & sizeof(off_t) == 4 ABI, but i wouldn't be
surprised if it showed up.  mips o64 or something wonky.

afaik, the exact output of the preprocessor isn't specified by a
standard, and it has changed subtly in the past.  so i wouldn't rely
on it in general.  something like `echo __SIZEOF_LONG__ | $(CPP) -E -P
- | tail -n1` would be a lot more reliable -- you aren't sniffing
preprocessor internals, you're actually preprocessing something.

POSIX does provide (optional) defines to try and sniff off_t sizes too:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/unistd.h.html

but i think the takeaway is that this is all not worth the effort, and
defining _FILE_OFFSET_BITS=64 all the time is way easier.
-mike

Steven Rostedt Dec. 20, 2022, 9 p.m. UTC | #2

On Tue, 20 Dec 2022 14:36:11 -0500
Mike Frysinger <[e-mail]> wrote:

> On Mon, Dec 19, 2022 at 5:34 PM Steven Rostedt wrote:
> > +# Make sure 32 bit on 64 bit doesn't have stat() bugs.  
> 
> i don't know what you're trying to say with "32 bit on 64 bit".  you
> mean a 32 bit binary (which ABI?) with a 64 bit kernel ?  or something
> else ?
> 
> the problem is entirely with using 32 bit filesystem interfaces.
> there are no other runtime requirements, either in the userland or
> kernel land.  filesystems moved to 64 bit fields in a number of places
> long ago regardless of the userland or kernel ABIs.
> 
> saying "32 bit" or "64 bit" is imprecise.  is x86_64's x32 & aarch64's
> ilp32 ABI "32 bit" ?  or is it "64 bit" ?  what about Window's x64
> (LLP64) ?
> https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
> https://unix.org/version2/whatsnew/lp64_wp.html
> 
> usually it's better to specify ABI names and/or specific types so
> people know what you're trying to cater to.

You are technically correct. Practically speaking, 32 bit architectures are
now mostly for niche devices that are most likely never going to see this
issue. My main fear was for 32 bit applications running on 64 bit kernels
where this is much more likely to be an issue. But I agree, I shouldn't
have made it sound like that's the only place it is an issue.

> 
> > +ifeq ($(shell echo '' | $(CPP) -dM - | grep __SIZEOF_LONG__ | cut -d' ' -f3),4)
> > +       CFLAGS += -D_FILE_OFFSET_BITS=64
> > +endif  
> 
> why do you need to sniff the size of long ?  -D_FILE_OFFSET_BITS=64
> should work fine regardless of the target ABI.
> 
> especially because you're assuming sizeof(long) == sizeof(off_t) which
> is not guaranteed anywhere.  on newer ABIs, like x32, sizeof(off_t) is
> always 64-bit while sizeof(long) is 32-bit.  i don't know if there's a
> sizeof(long) == 8 & sizeof(off_t) == 4 ABI, but i wouldn't be
> surprised if it showed up.  mips o64 or something wonky.
> 
> afaik, the exact output of the preprocessor isn't specified by a
> standard, and it has changed subtly in the past.  so i wouldn't rely
> on it in general.  something like `echo __SIZEOF_LONG__ | $(CPP) -E -P
> - | tail -n1` would be a lot more reliable -- you aren't sniffing
> preprocessor internals, you're actually preprocessing something.
> 
> POSIX does provide (optional) defines to try and sniff off_t sizes too:
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/unistd.h.html
> 
> but i think the takeaway is that this is all not worth the effort, and
> defining _FILE_OFFSET_BITS=64 all the time is way easier.
> -mike

Sure. I have no problem doing it for all archs.

BTW, the only issue that will happen if stat fails, is that a feature will
be disabled.

-- Steve

Mike Frysinger Dec. 20, 2022, 9:29 p.m. UTC | #3

On Tue, Dec 20, 2022 at 4:00 PM Steven Rostedt wrote:
> BTW, the only issue that will happen if stat fails, is that a feature will
> be disabled.

i am certainly by no means an expert in this codebase.  from glancing
at it, the impact is that people won't be able to load any plugins
right ?  that's where stat() & readdir() appear to be used (the dirent
returned by readdir has both ino_t & off_t in it).
-mike

Steven Rostedt Dec. 20, 2022, 9:35 p.m. UTC | #4

On Tue, 20 Dec 2022 16:29:49 -0500
Mike Frysinger <[e-mail]> wrote:

> i am certainly by no means an expert in this codebase.  from glancing
> at it, the impact is that people won't be able to load any plugins
> right ?  that's where stat() & readdir() appear to be used (the dirent
> returned by readdir has both ino_t & off_t in it).

If the plugins are not added, it will just not format some of the trace
events properly.

The plugins are used for better pretty printing the data, otherwise it just
prints out the field contents in their raw formats.

-- Steve

Patch

diff --git a/Makefile b/Makefile
index 965ff47eae7d..fd324033f6d7 100644
--- a/Makefile
+++ b/Makefile
@@ -23,6 +23,7 @@  endef
 $(call allow-override,CC,$(CROSS_COMPILE)gcc)
 $(call allow-override,AR,$(CROSS_COMPILE)ar)
 $(call allow-override,NM,$(CROSS_COMPILE)nm)
+$(call allow-override,CPP,$(CROSS_COMPILE)cpp)
 $(call allow-override,PKG_CONFIG,pkg-config)
 $(call allow-override,LD_SO_CONF_PATH,/etc/ld.so.conf.d/)
 $(call allow-override,LDCONFIG,ldconfig)
@@ -130,6 +131,11 @@  else
   CFLAGS := -g -Wall
 endif
 
+# Make sure 32 bit on 64 bit doesn't have stat() bugs.
+ifeq ($(shell echo '' | $(CPP) -dM - | grep __SIZEOF_LONG__ | cut -d' ' -f3),4)
+	CFLAGS += -D_FILE_OFFSET_BITS=64
+endif
+
 LIBS ?= -ldl
 export LIBS