[Toybox] sizeof(toybuf)

[enh]

On Wed, Apr 19, 2023 at 8:44 PM Rob Landley <rob at landley.net> wrote:

> On 4/19/23 16:02, enh wrote:
> > On Fri, Apr 14, 2023 at 8:49 PM Rob Landley <rob at landley.net
> > <mailto:rob at landley.net>> wrote:
> >
> >     On 4/14/23 18:49, enh wrote:
> >     >     > i don't like assert(), but **static_assert** is really
> useful for
> >     things like
> >     >     > this where you want to say "this code makes an assumption
> that you
> >     can test at
> >     >     > compile time".
> >     >
> >     >     Compile time is the time to care about that sort of thing, yes.
> >     >
> >     >     Kinda wonder about portability on weirdness like qnx (or the
> guy in
> >     email who's
> >     >     asking about uclibc-ng). I suspect any such asserts would be a
> >     CFG_DEBUG option
> >     >     maybe in portability.c? Hmmm...
> >     >
> >     > they haven't made other complaints about C11, so presumably
> static_assert is
> >     > fine too?
> >
> >     I'm not worried about it erroring, I'm worried about it potentially
> triggering
> >     in who knows what linker environment possibly producing mach-o
> binaries.
> >
> > i think you've mixed up two different threads here...
> >
> > static_assert is a compile-time check. there's currently a dependency in
> the
> > checksum code on sizeof(toybox) being < 5k-ish.
>
> Really? Checking a compile time constant at runtime doesn't sound like my
> style.
> (As in I'd have removed it during cleanup if I'd noticed, unless I
> _expect_ the
> test to optimize out and be a de-facto assert.)
>

you're the one who forked this from the original discussion on the specific
line of code in question :-P

https://github.com/landley/toybox/commit/aa88571a6b847a96bb8ee998a9868c5a1bdb3a6e#r108474092

the whole reason i suggested static_assert is because at the moment all the
"protection" we have is a comment.


> > static_assert can help you tell
> > the compiler to fail at compile time if that's not true. (and nothing
> ends up in
> > the binary.)
>
> I could have done #if sizeof(walrus) > 1234 around #error in
> portability.[ch]
> years ago if I'd needed to, the assert presumably is just syntactic sugar
> for
> that.


correct.


> But I'm unaware of such a check...?
>

exactly :-P

to go back to the beginning since this thread got far too confused: "if you
don't do this because the #if .. #error dance is too annoying/verbose,
static_assert lets you say this very concisely, and you're already
requiring C11 anyway".


> Checksum code... Do you mean libbuf? Not seeing such a check...
>
> There's a check that ping -s is small enough to fit in toybuf but that's
> the
> optstr providing a boundary for a command line argument... Lots of little
> implicit limitations due to using toybuf, such as ls -C maxing out at 1024
> columns...
>
> Not spotting it with a grep that includes sizeof. It could be a variable
> assignment that then gets tested later, but I'm not finding it at the
> moment.
>
> > alignas, on the other hand, _does_ cause changes to the elf/mach-o
> files. but
> > apple switched to clang before we did :-) (i also tested just now ---
> alignas
> > works as expected on macOS.)
>
> Ah, you're proposing the assert without the alignas. Presumably to
> (somehow?)
> check that none of the libc structs I'm throwing in toybuf gets bigger than
> toybuf because of library version skew. (Is there an easier way than
> putting an
> assert on every single such assignment? I generally check them and don't
> use
> toybox if it would take up half of it, and I also tend to check if the
> size of
> the glibc one, which is GOING to be the pig, has changed in the past 15
> years...)
>
> Let's see, demo_editline throws struct termios in there, httpd and netcat
> are
> using it as struct sockaddr, ping is using it as struct icmphdr... Ah,
> there's a
> pingchksum() using toybuf, is that what you meant? Again, that size check
> is on
> the size of the command line argument supplied to this run...
>
> All the sizeof(toybuf) tests SHOULD be comparing against something that
> varies
> at runtime...
>
> >     Sigh. I tried to make it gracefully fall back, and you guys are
> sending kill
> >     signals to anything that uses a system call you don't recognize.
> >
> > like i say every time you complain: because the alternative was worse. we
> > started off just failing the syscalls, but *way* too much code doesn't
> check, or
> > has untested error paths that don't actually work, or has error paths
> that do
> > "work" but just hide the failure and make it impossible to debug. if
> you're
> > trying to do this for a single project, that would be an option. for an
> entire
> > operating system, not so much.
>
> Indeed. You're shipping a billion unadministered unix systems per year with
> full-room audio pickup, GPS tracking, and a camera pointed at the user's
> face in
> normal operation, through which their users regularly do financial
> transactions.
>
> Your paranoia level is quite understandably a bit high.
>
> >     > if we want to go this route, we'll probably have to go with
> something like a
> >     > signal handler for SIGSYS and a boolean. or a weak declaration of
> the
> >     > copy_file_range() *function*? no, because that's useless for the
> static
> >     linking
> >     > we're concerned about.
> >     >
> >     > oh, wait... we should just be able to
> use android_get_device_api_level()...
> >     >
> >     > i think something like this should work?
> >
> >     Sigh, lemme finish and check in the fs_type_name() redo. (So many
> dirty files in
> >     my tree. Closing tabs...)
> >
> > well, i should try to test this first... :-) if nothing else, it
> occurred to me
> > since sending it that we shouldn't check the system property on _every_
> call,
> > and should cache that.
>
> Ready to test a patch when you've got an update...
>
> Rob
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20230420/67d1eca7/attachment.htm>