On Fri, May 28, 2021 at 04:48:04PM +0200, Hiltjo Posthuma wrote:
> On Fri, May 28, 2021 at 11:25:43AM +0100, Stuart Henderson wrote:
> > On 2021/05/28 12:14, Hiltjo Posthuma wrote:
> > > On Thu, Feb 04, 2021 at 01:26:44PM +0100, Hiltjo Posthuma wrote:
> > > > On Tue, Jan 26, 2021 at 08:27:24PM +0100, Hiltjo Posthuma wrote:
> > > > > On Tue, Jan 26, 2021 at 03:56:11PM +0000, Stuart Henderson wrote:
> > > > > > On 2021/01/26 15:31, Clemens Gößnitzer wrote:
> > > > > > > January 26, 2021 3:44 PM, "Hiltjo Posthuma"
> > > > > > > <hil...@codemadness.org> wrote:
> > > > > > > > On Sat, Jan 16, 2021 at 04:29:27PM +0100, Hiltjo Posthuma wrote:
> > > > > > > >> On Mon, Jan 11, 2021 at 07:50:55PM +0100, Hiltjo Posthuma
> > > > > > > >> wrote:
> > > > > > > >>
> > > > > > > >> The below patch pledges the iconv binary in the libiconv
> > > > > > > >> package. The tool is
> > > > > > > >> useful for converting text-encoding of text data to UTF-8 for
> > > > > > > >> example.
> > > > > > > >>
> > > > > > > >> It now uses pledge("stdio", NULL) if only using stdin/stdout.
> > > > > > > >> It uses
> > > > > > > >> pledge("stdio rpath", NULL) when specifying files.
> > > > > > > >>
> > > > > > > >> I've tested many command-line option combinations and haven't
> > > > > > > >> found missing
> > > > > > > >> promises which cause an abort().
> > > > > > > >>
> > > > > > > >> Patch:
> > > > > > ..
> > > > > > > >> +@@ -846,6 +849,9 @@
> > > > > > > >> + struct iconv_hooks hooks;
> > > > > > > >> + int i;
> > > > > > > >> + int status;
> > > > > > > >> ++
> > > > > > > >> ++ if (pledge(i == argc ? "stdio" : "stdio rpath", NULL) == -1)
> > > > > > >
> > > > > > > Wouldn't you use i uninitialised here?
> > > > > > >
> > > > > > > >> ++ err(1, "pledge");
> > > > > > > >> +
> > > > > > > >> + set_program_name (argv[0]);
> > > > > > > >> + #if HAVE_SETLOCALE
> > > > > > > >> --
> > > > > >
> > > > > > Yes, it needs to be done after parsing the arguments in the loop
> > > > > > after
> > > > > > calling textdomain().
> > > > > >
> > > > > > Looks like it was previously done like that but moved before
> > > > > > sending out
> > > > > > the diff? I assume it was moved so that more of the code was moved
> > > > > > under
> > > > > > pledge. Better approach might be to unconditionally pledge stdio
> > > > > > rpath,
> > > > > > then, after the loop, conditionally pledge again to drop rpath if
> > > > > > possible.
> > > > > >
> > > > > > It would be nicer to use the error function used in the rest of
> > > > > > the file rather than pulling in another header for err().
> > > > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > Thanks both for the review! I updated the changes in the patch below.
> > > > > It was indeed a mistake in creating the patch, I'm sorry for the
> > > > > sloppiness.
> > > > >
> > > > >
> > > > > From dbb04c280d8ca368da43c0fdf185c3e9a4a59050 Mon Sep 17 00:00:00 2001
> > > > > From: Hiltjo Posthuma <hil...@codemadness.org>
> > > > > Date: Tue, 26 Jan 2021 20:21:32 +0100
> > > > > Subject: [PATCH] libiconv: pledge iconv(1) binary
> > > > >
> > > > > ---
> > > > > converters/libiconv/Makefile | 3 +-
> > > > > converters/libiconv/patches/patch-src_iconv_c | 29
> > > > > +++++++++++++++++++
> > > > > 2 files changed, 31 insertions(+), 1 deletion(-)
> > > > > create mode 100644 converters/libiconv/patches/patch-src_iconv_c
> > > > >
> > > > > diff --git a/converters/libiconv/Makefile
> > > > > b/converters/libiconv/Makefile
> > > > > index 2ab58ea4519..5c8043270de 100644
> > > > > --- a/converters/libiconv/Makefile
> > > > > +++ b/converters/libiconv/Makefile
> > > > > @@ -5,7 +5,7 @@ COMMENT= character set conversion library
> > > > > DISTNAME= libiconv-1.16
> > > > > CATEGORIES= converters devel
> > > > > MASTER_SITES= ${MASTER_SITE_GNU:=libiconv/}
> > > > > -REVISION= 0
> > > > > +REVISION= 1
> > > > >
> > > > > SHARED_LIBS= charset 1.1 \
> > > > > iconv 7.0
> > > > > @@ -17,6 +17,7 @@ MAINTAINER= Brad Smith <b...@comstyle.com>
> > > > > # LGPLv2 and GPLv3
> > > > > PERMIT_PACKAGE= Yes
> > > > >
> > > > > +# uses pledge()
> > > > > WANTLIB= c
> > > > >
> > > > > SEPARATE_BUILD= Yes
> > > > > diff --git a/converters/libiconv/patches/patch-src_iconv_c
> > > > > b/converters/libiconv/patches/patch-src_iconv_c
> > > > > new file mode 100644
> > > > > index 00000000000..9b673fbe5db
> > > > > --- /dev/null
> > > > > +++ b/converters/libiconv/patches/patch-src_iconv_c
> > > > > @@ -0,0 +1,29 @@
> > > > > +--- src/iconv.c.orig Fri Apr 26 20:50:13 2019
> > > > > ++++ src/iconv.c Tue Jan 26 20:07:34 2021
> > > > > +@@ -19,6 +19,8 @@
> > > > > + # define ICONV_CONST
> > > > > + #endif
> > > > > +
> > > > > ++#include <unistd.h>
> > > > > ++
> > > > > + #include <limits.h>
> > > > > + #include <stddef.h>
> > > > > + #include <stdio.h>
> > > > > +@@ -847,6 +849,8 @@
> > > > > + int i;
> > > > > + int status;
> > > > > +
> > > > > ++ if (pledge("stdio rpath", NULL) == -1)
> > > > > ++ error(EXIT_FAILURE, errno, "pledge");
> > > > > + set_program_name (argv[0]);
> > > > > + #if HAVE_SETLOCALE
> > > > > + /* Needed for the locale dependent encodings, "char" and
> > > > > "wchar_t",
> > > > > +@@ -1002,6 +1006,8 @@
> > > > > + }
> > > > > + break;
> > > > > + }
> > > > > ++ if ((do_list || i == argc) && pledge("stdio", NULL) == -1)
> > > > > ++ error(EXIT_FAILURE, errno, "pledge");
> > > > > + if (do_list) {
> > > > > + if (i != 2 || i != argc)
> > > > > + usage(1);
> > > > > --
> > > > > 2.30.0
> > > > >
> > > >
> > > > Last bump for this from me, any OKs to pledge this port?
> > > >
> > >
> > > Another bump from me I guess. It would be nice for me to have this
> > > (POSIX)
> > > command-line binary pledge'ed.
> > >
> > > Any OKs or comments?
> > >
> > > --
> > > Kind regards,
> > > Hiltjo
> > >
> >
> > I think this is generally sound, can you resend with a usable
> > diff so we don't need to dig it out of the list archives, and
> > cc the maintainer please?
> >
>
> Hi,
>
> Below is the latest version of the patch:
>
>
> diff --git a/converters/libiconv/Makefile b/converters/libiconv/Makefile
> index 2ab58ea4519..5c8043270de 100644
> --- a/converters/libiconv/Makefile
> +++ b/converters/libiconv/Makefile
> @@ -5,7 +5,7 @@ COMMENT= character set conversion library
> DISTNAME= libiconv-1.16
> CATEGORIES= converters devel
> MASTER_SITES= ${MASTER_SITE_GNU:=libiconv/}
> -REVISION= 0
> +REVISION= 1
>
> SHARED_LIBS= charset 1.1 \
> iconv 7.0
> @@ -17,6 +17,7 @@ MAINTAINER= Brad Smith <b...@comstyle.com>
> # LGPLv2 and GPLv3
> PERMIT_PACKAGE= Yes
>
> +# uses pledge()
> WANTLIB= c
>
> SEPARATE_BUILD= Yes
> diff --git a/converters/libiconv/patches/patch-src_iconv_c
> b/converters/libiconv/patches/patch-src_iconv_c
> new file mode 100644
> index 00000000000..9b673fbe5db
> --- /dev/null
> +++ b/converters/libiconv/patches/patch-src_iconv_c
> @@ -0,0 +1,29 @@
> +--- src/iconv.c.orig Fri Apr 26 20:50:13 2019
> ++++ src/iconv.c Tue Jan 26 20:07:34 2021
> +@@ -19,6 +19,8 @@
> + # define ICONV_CONST
> + #endif
> +
> ++#include <unistd.h>
> ++
> + #include <limits.h>
> + #include <stddef.h>
> + #include <stdio.h>
> +@@ -847,6 +849,8 @@
> + int i;
> + int status;
> +
> ++ if (pledge("stdio rpath", NULL) == -1)
> ++ error(EXIT_FAILURE, errno, "pledge");
> + set_program_name (argv[0]);
> + #if HAVE_SETLOCALE
> + /* Needed for the locale dependent encodings, "char" and "wchar_t",
> +@@ -1002,6 +1006,8 @@
> + }
> + break;
> + }
> ++ if ((do_list || i == argc) && pledge("stdio", NULL) == -1)
> ++ error(EXIT_FAILURE, errno, "pledge");
> + if (do_list) {
> + if (i != 2 || i != argc)
> + usage(1);
>
> --
> Kind regards,
> Hiltjo
>
Bump... any OKs?
--
Kind regards,
Hiltjo
