Re: [new] devel/libfixposix
Hi Ingo, I'm sorry for the delay. I started looking into it, but then I was distracted by something and forgot to reply, please apologies. Ingo Schwarze writes: > Hi Omar, > > Omar Polo wrote on Fri, Dec 03, 2021 at 08:00:03PM +0100: > >> Woops, I identified a correct issue in unistd.c, but the fix was wrong. >> Updated tarball attached and upstream notified :) >> >> (update: upstream has accepted one of the two patches) >> >> Also enabling the built-in tests this time, I haven't noticed them the >> first time because without --enable-tests `make test' was successful and >> listed 0 tests available. (The mkstemp test fails because why check >> what malloc returns?) > > Failing to check return values is bad, too, but not the root cause of > the failure. That test is attempting to allocate (2^24 - 1) * 15 = > 251658225 bytes of memory in a single malloc(3) call, about 240 MB, > because on OpenBSD, TMP_MAX is 0x7fff. Running that particular > test makes no sense until upstream fixes it. > > The test src/tests/buildinfo.bats is also broken. It assumes that, > if you have a file "./utility" that start with the first line > "#!/bin/sh" and a file "./argument" that starts with the first > line "#!./utility" and then run "./argument", that the kernel > would run "#!/bin/sh ./utility ./argument". But our kernel > runs "#!/bin/sh ./argument" instead. thanks for figuring this out! the mkstemp actually passes with a lower NTEMPFILES, but it's pointless anyway as all it does is testing our mkostemp(3), but for the buildinfo test I was a bit clueless (I haven't throwed at it too much time neither to be honest) > The buildinfo.bats issue is more or less unfixable because on the > one hand, the automake infrastructure in Makefile.am and the > script config/aux/tap-driver.sh require that the TESTS make > variable must contain the name of the test *data* file, but then > turn around and tell the kernel to *execute* that data file. > To make that portable, upstream needs to completely restructure > that maze of indirections and abstractions. > > On top of that, the tests require gmake(1) and bash(1) while > nothing in the actual software requires those. > > So half the tests (2 out of 4) are broken and completely non-portable. > The "select" test is completely trivial and certainly does not test > select(2) in any meaningful way. So what remains as potentially > working and non-trivial is one single test, the "spawn" test. I don't > think that is worth depending on gmake and bash and all the trouble > i described above. Consequently, i suggest to just forget about the > tests until upstream gets their shit together and makes sure they > are portable, work, and test some non-trivial features. > > I mean, come on, this is supposed to be a portability library - > but all the same, the test suite is thoroughly Linux-only... I agree on everything. It all started with a small library to allow a common lisp extension for x11/stumpwm to work, but more I look into the project itself the less I want to have it on my machine. I'm sorry for wasting your time too on this. > [...] > * Add CONFIGURE_ENV += ac_cv_prog_GIT= >That achieves the same as patch-config_m4_custom_checks_m4 , >but in a much simpler way. Do forget to remove that patch >if you agree with this change. (thanks for remembering me of the ac_cv_* variables. I always forget that it's possible to override the checks that way) > * Add a comment explaining why the test suite makes no sense >just yet, with an incomplete list of changes that would be >needed to enable it, all commented out, such that we don't >forget what we already found out. > > * Add a comment about the autotools versions. >I think someone should talk to upstream and find out >which versions this software is actually designed for. > > Feel free to use part or all of this in any way you want. > I don't want to commit it, but you might. :-) I agree on everything :) I'll make sure to report upstream every issue mentioned and work with them until they're resolved, but for the port itself I'm less and less sure I want to add it, at least in the current state. Thanks, Omar Polo > Yours, > Ingo > > > # $OpenBSD$ > > COMMENT = thin wrapper over libc functions and macros > > GH_ACCOUNT = sionescu > GH_PROJECT = libfixposix > GH_TAGNAME = v0.4.3 > > SHARED_LIBS += fixposix 0.0 # 5.0 > > CATEGORIES = devel > > MAINTAINER = Omar Polo > > # Boost SL 1.0 > PERMIT_PACKAGE = Yes > > WANTLIB += pthread > > SEPARATE_BUILD = Yes > CONFIGURE_STYLE = autoreconf > CONFIGURE_ENV += ac_cv_prog_GIT= > > # The test suite is almost trivial, quite non-portable, > # and utterly broken, so leave it disabled for now. > # There are only four tests: two are broken and one is trivial. > # The test suite requires gmake and bash. > # The presence of the check(1) utility is tested by ./configure > # unless the following magical incantation
Re: [new] devel/libfixposix
Hi Omar, Omar Polo wrote on Fri, Dec 03, 2021 at 08:00:03PM +0100: > Woops, I identified a correct issue in unistd.c, but the fix was wrong. > Updated tarball attached and upstream notified :) > > (update: upstream has accepted one of the two patches) > > Also enabling the built-in tests this time, I haven't noticed them the > first time because without --enable-tests `make test' was successful and > listed 0 tests available. (The mkstemp test fails because why check > what malloc returns?) Failing to check return values is bad, too, but not the root cause of the failure. That test is attempting to allocate (2^24 - 1) * 15 = 251658225 bytes of memory in a single malloc(3) call, about 240 MB, because on OpenBSD, TMP_MAX is 0x7fff. Running that particular test makes no sense until upstream fixes it. The test src/tests/buildinfo.bats is also broken. It assumes that, if you have a file "./utility" that start with the first line "#!/bin/sh" and a file "./argument" that starts with the first line "#!./utility" and then run "./argument", that the kernel would run "#!/bin/sh ./utility ./argument". But our kernel runs "#!/bin/sh ./argument" instead. The buildinfo.bats issue is more or less unfixable because on the one hand, the automake infrastructure in Makefile.am and the script config/aux/tap-driver.sh require that the TESTS make variable must contain the name of the test *data* file, but then turn around and tell the kernel to *execute* that data file. To make that portable, upstream needs to completely restructure that maze of indirections and abstractions. On top of that, the tests require gmake(1) and bash(1) while nothing in the actual software requires those. So half the tests (2 out of 4) are broken and completely non-portable. The "select" test is completely trivial and certainly does not test select(2) in any meaningful way. So what remains as potentially working and non-trivial is one single test, the "spawn" test. I don't think that is worth depending on gmake and bash and all the trouble i described above. Consequently, i suggest to just forget about the tests until upstream gets their shit together and makes sure they are portable, work, and test some non-trivial features. I mean, come on, this is supposed to be a portability library - but all the same, the test suite is thoroughly Linux-only... Next problem to consider for upstream: when run with automake 1.16 and autoconf 2.71, the build complains loudly: configure.ac:53: warning: The macro `AC_PROG_CC_C99' is obsolete. configure.ac:53: You should run autoupdate. /usr/local/share/autoconf-2.71/autoconf/c.m4:1662: AC_PROG_CC_C99 is expanded from... configure.ac:53: the top level configure.ac:67: warning: The macro `AC_LANG_C' is obsolete. configure.ac:67: You should run autoupdate. /usr/local/share/autoconf-2.71/autoconf/c.m4:72: AC_LANG_C is expanded from... config/m4/ax_pthread.m4:283: AX_PTHREAD is expanded from... configure.ac:67: the top level configure.ac:67: warning: The macro `AC_TRY_LINK' is obsolete. configure.ac:67: You should run autoupdate. /usr/local/share/autoconf-2.71/autoconf/general.m4:2921: AC_TRY_LINK is expanded from... config/m4/ax_pthread.m4:283: AX_PTHREAD is expanded from... configure.ac:67: the top level So that is apparently newer than the autotools versions they are aiming for. But when trying to use the autotools versions advertised in README.md, automake-1.10 and autoconf-2.67, the build crashes instantly, so those are apparently too old. There are more problems with the software. For example, in src/lib/install_signalfd.c, i spotted a signal handler that calls snprintf(3). I don't doubt there is more. Then again, correctness of upstream code isn't really a requirement for a port... I'm suggesting the version of the Makefile appended below, with the following changes: * Remove BUILD_DEPENDS = devel/check The build does not need it, only the tests might, some day. * Remove USE_GMAKE = Yes Again, the build does not need it, only the tests do. * Add SEPARATE_BUILD = Yes Upstream strongly recommends that, and faq/ports/testing.html recommends it, too. * Add CONFIGURE_ENV += ac_cv_prog_GIT= That achieves the same as patch-config_m4_custom_checks_m4 , but in a much simpler way. Do forget to remove that patch if you agree with this change. * Add a comment explaining why the test suite makes no sense just yet, with an incomplete list of changes that would be needed to enable it, all commented out, such that we don't forget what we already found out. * Add a comment about the autotools versions. I think someone should talk to upstream and find out which versions this software is actually designed for. Feel free to use part or all of this in any way you want. I don't want to commit it, but you might. :-) Yours, Ingo # $OpenBSD$ COMMENT = thin wrapper over libc functions and macros GH_ACCOUNT =
Re: [new] devel/libfixposix
Woops, I identified a correct issue in unistd.c, but the fix was wrong.
Updated tarball attached and upstream notified :)
(update: upstream has accepted one of the two patches)
Also enabling the built-in tests this time, I haven't noticed them the
first time because without --enable-tests `make test' was successful and
listed 0 tests available. (The mkstemp test fails because why check
what malloc returns?)
Omar Polo writes:
> Ingo Schwarze writes:
>
>> Hi Omar,
>
> Hello Ingo,
>
>> Omar Polo wrote on Fri, Dec 03, 2021 at 09:07:02AM +0100:
>>
>>> % pkg_info libfixposix
>>> Information for inst:libfixposix-0.4.3
>>>
>>> Comment:
>>> thin wrapper over POSIX syscalls
>>>
>>> Description:
>>> The purpose of libfixposix is to offer replacements for parts of POSIX
>>> whose behaviour is inconsistent across *NIX flavours.
>>
>> Without looking at the code:
>> This sounds totally scary to me.
>>
>> Wouldn't it be better to provide a dummy instead that just passes
>> the calls through?
>>
>> I mean, decisions about OpenBSD deviating from POSIX are usually
>> made for a reason, aren't they? For example, the first things
>> coming to my mind are rand(3) and gets(3) and printf(%n), and i feel
>> certain there are several more.
>>
>> Software depending on POSIX behaviour that we intentionally disabled
>> should not just patch it back, right? I think it is better to have
>> such software fail to build (or even, admittedly less conveniently,
>> crash at run time).
>>
>> Then, the actual problems can be investigated and fixed properly.
>>
>> Wouldn't this port essentially implement security mitigation
>> countermeasures, as the famous saying by tedu@ goes?
>>
>> This is not an objection, just a question.
>
> I agree with your sentiment, and in fact I was a bit scared too the
> first time I heard of the library, but upon a further look it turns out
> not to be the case fortunately.
>
> What this library does is wrapping most of the functionality of libc, in
> particular macros and constants, behind functions calls. This is needed
> for some higher level language where is impossible or particularly
> cumbersome to access those. One example may be src/lib/select.c:
>
> 55 DSO_PUBLIC void
> 56 lfp_fd_set(int fd, fd_set *set)
> 57 {
> 58 FD_SET(fd, set);
> 59 }
> 60
> 61 DSO_PUBLIC void
> 62 lfp_fd_zero(fd_set *set)
> 63 {
> 64 FD_ZERO(set);
> 65 }
>
> One may argue that it's the fault of the language that doesn't provide
> such wrapper -or equivalent- by itself, and I would agree, but
> unfortunately that's the case. Functions like those allows, for
> example, a Common Lisp program to use select(2).
>
> Most of the content of the library is just wrapping functions from libc
> for (apparently) no reason at all. Take src/lib/resource.c for example:
>
> 27 DSO_PUBLIC int
> 28 lfp_getrlimit(int resource, struct rlimit *rlim)
> 29 {
> 30 return getrlimit(resource, rlim);
> 31 }
>
> So there's nothing like a rand(3) implementation or a different
> printf(3) that allows %n.
>
> I did, however, sit down and read the whole thing from top to bottom
> (the first time I did only a generic scan.) It's not what I would call
> a pretty library, but I think it's innocuous at worst. I zapped a
> "mkostemp NIH" which is, I think, the most grave offender. (the second
> place goes to the "almost" posix_spawn rewrite in src/lib/spawn.c, but
> it's slightly different from posix_spawn and would avoid to touch that
> if possible.)
>
> I'm also starting to think that DESCR needs some tweaking because even
> if that's how upstream describes it, it's not really correct. True,
> there is some platform-dependent code (in particular for macos, but also
> a dummy sendfile(2) for example) but it's still misleading, and your
> mail convinced me of that. pkg/DESCR now reads
>
>> libfixposix provides some of the functionalities from libc behind
>> function calls thus allowing programs written in higher level
>> languages to access things that are usually hidden behind macros or
>> defines.
>
> and the comment is
>
>> thin wrapper over libc functions and macros
>
> I hope it would clear some doubts, but I'm open to improvements (as you
> may have already noted, I'm not a skilled writer ;-)
>
>>> Maintainer: The OpenBSD ports mailing-list
>>
>> For such a scary beast, shouldn't there at least be a maintainer
>> who does basic auditing of the library, making sure that it does
>> not cause a security disaster, and making sure that future versions
>> do not introduce vulnerabilities either?
>>
>> To me, this feels like sending a Mastiff to promenade alone in the
>> park instead of leading it on a leash?
>
> I agree on this too, and if nobody objects I'll take MAINTAINER on this
> port.
>
> I'm attaching an updated tarball which fixes a couple of things I
> discovered upon closer look. I'll also try to upstream the patches.
>
>> Yours,
>> Ingo
>
> Tha
Re: [new] devel/libfixposix
Ingo Schwarze writes:
> Hi Omar,
Hello Ingo,
> Omar Polo wrote on Fri, Dec 03, 2021 at 09:07:02AM +0100:
>
>> % pkg_info libfixposix
>> Information for inst:libfixposix-0.4.3
>>
>> Comment:
>> thin wrapper over POSIX syscalls
>>
>> Description:
>> The purpose of libfixposix is to offer replacements for parts of POSIX
>> whose behaviour is inconsistent across *NIX flavours.
>
> Without looking at the code:
> This sounds totally scary to me.
>
> Wouldn't it be better to provide a dummy instead that just passes
> the calls through?
>
> I mean, decisions about OpenBSD deviating from POSIX are usually
> made for a reason, aren't they? For example, the first things
> coming to my mind are rand(3) and gets(3) and printf(%n), and i feel
> certain there are several more.
>
> Software depending on POSIX behaviour that we intentionally disabled
> should not just patch it back, right? I think it is better to have
> such software fail to build (or even, admittedly less conveniently,
> crash at run time).
>
> Then, the actual problems can be investigated and fixed properly.
>
> Wouldn't this port essentially implement security mitigation
> countermeasures, as the famous saying by tedu@ goes?
>
> This is not an objection, just a question.
I agree with your sentiment, and in fact I was a bit scared too the
first time I heard of the library, but upon a further look it turns out
not to be the case fortunately.
What this library does is wrapping most of the functionality of libc, in
particular macros and constants, behind functions calls. This is needed
for some higher level language where is impossible or particularly
cumbersome to access those. One example may be src/lib/select.c:
55 DSO_PUBLIC void
56 lfp_fd_set(int fd, fd_set *set)
57 {
58 FD_SET(fd, set);
59 }
60
61 DSO_PUBLIC void
62 lfp_fd_zero(fd_set *set)
63 {
64 FD_ZERO(set);
65 }
One may argue that it's the fault of the language that doesn't provide
such wrapper -or equivalent- by itself, and I would agree, but
unfortunately that's the case. Functions like those allows, for
example, a Common Lisp program to use select(2).
Most of the content of the library is just wrapping functions from libc
for (apparently) no reason at all. Take src/lib/resource.c for example:
27 DSO_PUBLIC int
28 lfp_getrlimit(int resource, struct rlimit *rlim)
29 {
30 return getrlimit(resource, rlim);
31 }
So there's nothing like a rand(3) implementation or a different
printf(3) that allows %n.
I did, however, sit down and read the whole thing from top to bottom
(the first time I did only a generic scan.) It's not what I would call
a pretty library, but I think it's innocuous at worst. I zapped a
"mkostemp NIH" which is, I think, the most grave offender. (the second
place goes to the "almost" posix_spawn rewrite in src/lib/spawn.c, but
it's slightly different from posix_spawn and would avoid to touch that
if possible.)
I'm also starting to think that DESCR needs some tweaking because even
if that's how upstream describes it, it's not really correct. True,
there is some platform-dependent code (in particular for macos, but also
a dummy sendfile(2) for example) but it's still misleading, and your
mail convinced me of that. pkg/DESCR now reads
> libfixposix provides some of the functionalities from libc behind
> function calls thus allowing programs written in higher level
> languages to access things that are usually hidden behind macros or
> defines.
and the comment is
> thin wrapper over libc functions and macros
I hope it would clear some doubts, but I'm open to improvements (as you
may have already noted, I'm not a skilled writer ;-)
>> Maintainer: The OpenBSD ports mailing-list
>
> For such a scary beast, shouldn't there at least be a maintainer
> who does basic auditing of the library, making sure that it does
> not cause a security disaster, and making sure that future versions
> do not introduce vulnerabilities either?
>
> To me, this feels like sending a Mastiff to promenade alone in the
> park instead of leading it on a leash?
I agree on this too, and if nobody objects I'll take MAINTAINER on this
port.
I'm attaching an updated tarball which fixes a couple of things I
discovered upon closer look. I'll also try to upstream the patches.
> Yours,
> Ingo
Thanks!
Omar Polo
libfixposix.tar.gz
Description: Binary data
Re: [new] devel/libfixposix
Hi Omar, Omar Polo wrote on Fri, Dec 03, 2021 at 09:07:02AM +0100: > % pkg_info libfixposix > Information for inst:libfixposix-0.4.3 > > Comment: > thin wrapper over POSIX syscalls > > Description: > The purpose of libfixposix is to offer replacements for parts of POSIX > whose behaviour is inconsistent across *NIX flavours. Without looking at the code: This sounds totally scary to me. Wouldn't it be better to provide a dummy instead that just passes the calls through? I mean, decisions about OpenBSD deviating from POSIX are usually made for a reason, aren't they? For example, the first things coming to my mind are rand(3) and gets(3) and printf(%n), and i feel certain there are several more. Software depending on POSIX behaviour that we intentionally disabled should not just patch it back, right? I think it is better to have such software fail to build (or even, admittedly less conveniently, crash at run time). Then, the actual problems can be investigated and fixed properly. Wouldn't this port essentially implement security mitigation countermeasures, as the famous saying by tedu@ goes? This is not an objection, just a question. > Maintainer: The OpenBSD ports mailing-list For such a scary beast, shouldn't there at least be a maintainer who does basic auditing of the library, making sure that it does not cause a security disaster, and making sure that future versions do not introduce vulnerabilities either? To me, this feels like sending a Mastiff to promenade alone in the park instead of leading it on a leash? Yours, Ingo
Re: [new] devel/libfixposix
Stuart Henderson writes: > On 2021/12/03 10:20, Omar Polo wrote: >> Stuart Henderson writes: >> >> > On 2021/12/03 09:07, Omar Polo wrote: >> >> Hello ports, >> >> >> >> % pkg_info libfixposix >> >> Information for inst:libfixposix-0.4.3 >> >> >> >> Comment: >> >> thin wrapper over POSIX syscalls >> >> >> >> Description: >> >> The purpose of libfixposix is to offer replacements for parts of POSIX >> >> whose behaviour is inconsistent across *NIX flavours. >> >> >> >> Maintainer: The OpenBSD ports mailing-list >> >> >> >> WWW: https://github.com/sionescu/libfixposix >> >> >> >> >> >> there are some common lisp libraries that depends on this. I ported it >> >> around one year ago because I needed it for a x11/stumpwm extension, but >> >> there were issues with the common lisp library iolib. These issues were >> >> fixed recently, so here's the port. >> >> >> >> OK? >> >> >> >> P.S.: only one doubt: the port is written in C99 which should be ok for >> >> base-gcc right? >> >> >> > >> > : CONFIGURE_STYLE = gnu autoreconf >> > >> > s/gnu //, and please set AUTOCONF_VERSION/AUTOMAKE_VERSION to something >> > less >> > of an antique than the defaults. >> >> Sure! >> >> Here's an updated tarball with CONFIGURE_STYLE=autoreconf and >> >> AUTOCONF_VERSION=2.71 >> AUTOMAKE_VERSION=1.16 >> > > Nitpicking but I'm not a fan of mixing "VAR=" and "VAR =" in the same > Makefile, other than that OK with me. completely agree, I was in a hurry and copy-pasted the output of `env | grep -i auto' I'll import it later with the proper spacing and alignment, thanks :)
Re: [new] devel/libfixposix
On 2021/12/03 10:20, Omar Polo wrote: > Stuart Henderson writes: > > > On 2021/12/03 09:07, Omar Polo wrote: > >> Hello ports, > >> > >> % pkg_info libfixposix > >> Information for inst:libfixposix-0.4.3 > >> > >> Comment: > >> thin wrapper over POSIX syscalls > >> > >> Description: > >> The purpose of libfixposix is to offer replacements for parts of POSIX > >> whose behaviour is inconsistent across *NIX flavours. > >> > >> Maintainer: The OpenBSD ports mailing-list > >> > >> WWW: https://github.com/sionescu/libfixposix > >> > >> > >> there are some common lisp libraries that depends on this. I ported it > >> around one year ago because I needed it for a x11/stumpwm extension, but > >> there were issues with the common lisp library iolib. These issues were > >> fixed recently, so here's the port. > >> > >> OK? > >> > >> P.S.: only one doubt: the port is written in C99 which should be ok for > >> base-gcc right? > >> > > > > : CONFIGURE_STYLE = gnu autoreconf > > > > s/gnu //, and please set AUTOCONF_VERSION/AUTOMAKE_VERSION to something less > > of an antique than the defaults. > > Sure! > > Here's an updated tarball with CONFIGURE_STYLE=autoreconf and > > AUTOCONF_VERSION=2.71 > AUTOMAKE_VERSION=1.16 > Nitpicking but I'm not a fan of mixing "VAR=" and "VAR =" in the same Makefile, other than that OK with me.
Re: [new] devel/libfixposix
Stuart Henderson writes: > On 2021/12/03 09:07, Omar Polo wrote: >> Hello ports, >> >> % pkg_info libfixposix >> Information for inst:libfixposix-0.4.3 >> >> Comment: >> thin wrapper over POSIX syscalls >> >> Description: >> The purpose of libfixposix is to offer replacements for parts of POSIX >> whose behaviour is inconsistent across *NIX flavours. >> >> Maintainer: The OpenBSD ports mailing-list >> >> WWW: https://github.com/sionescu/libfixposix >> >> >> there are some common lisp libraries that depends on this. I ported it >> around one year ago because I needed it for a x11/stumpwm extension, but >> there were issues with the common lisp library iolib. These issues were >> fixed recently, so here's the port. >> >> OK? >> >> P.S.: only one doubt: the port is written in C99 which should be ok for >> base-gcc right? >> > > : CONFIGURE_STYLE = gnu autoreconf > > s/gnu //, and please set AUTOCONF_VERSION/AUTOMAKE_VERSION to something less > of an antique than the defaults. Sure! Here's an updated tarball with CONFIGURE_STYLE=autoreconf and AUTOCONF_VERSION=2.71 AUTOMAKE_VERSION=1.16 libfixposix.tar.gz Description: Binary data
Re: [new] devel/libfixposix
On 2021/12/03 09:07, Omar Polo wrote: > Hello ports, > > % pkg_info libfixposix > Information for inst:libfixposix-0.4.3 > > Comment: > thin wrapper over POSIX syscalls > > Description: > The purpose of libfixposix is to offer replacements for parts of POSIX > whose behaviour is inconsistent across *NIX flavours. > > Maintainer: The OpenBSD ports mailing-list > > WWW: https://github.com/sionescu/libfixposix > > > there are some common lisp libraries that depends on this. I ported it > around one year ago because I needed it for a x11/stumpwm extension, but > there were issues with the common lisp library iolib. These issues were > fixed recently, so here's the port. > > OK? > > P.S.: only one doubt: the port is written in C99 which should be ok for > base-gcc right? > : CONFIGURE_STYLE = gnu autoreconf s/gnu //, and please set AUTOCONF_VERSION/AUTOMAKE_VERSION to something less of an antique than the defaults.
