Re: [patch] UPDATE: converters/libiconv: pledge iconv binary

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" <hiltjo@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 <hiltjo@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 <brad@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
+