Re: mail/exim and printf("%n")

On 9/2/21 10:46 PM, Jeremie Courreges-Anglas wrote:
> On Thu, Sep 02 2021, "Theo de Raadt" <deraadt@openbsd.org> wrote:
>> Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:
>>
>>>
>>>
>>> exim apparently uses printf("%n"), which is currently forbidden (libc
>>> calls abort(3)).
>>>
>>> I don't want us to fix all the %n uses in the ports tree, but instead
>>> wait for user reports. Though for some software like exim it makes
>>> sense to help users avoid such a hard crash.
>>>
>>> The diff below doesn't pretend to fix all uses of %n in the exim source.
>>> There may be others that can't be flagged by the compiler (support for
>>> that hesn't been committed yet) because of indirections through wrapper
>>> functions.
>>> +--- src/acl.c.orig
>>> ++++ src/acl.c
>>> +@@ -2906,10 +2906,12 @@ for (; cb; cb = cb->next)
>>> +
>>> + HDEBUG(D_acl)
>>> + {
>>> +- int lhswidth = 0;
>>> +- debug_printf_indent("check %s%s %n",
>>> ++ uschar buf[256];
>>> ++ int lhswidth = snprintf(CS buf, sizeof buf, "check %s%s ",
>>> + (!conditions[cb->type].is_modifier && cb->u.negated)? "!":"",
>>> +- conditions[cb->type].name, &lhswidth);
>>> ++ conditions[cb->type].name);
>>> ++ if (lhswidth == -1) lhswidth = 0;
>>> ++ debug_printf_indent("%s");
>>
>> Doesn't this %s need an argument buf?
>
> Urkh, indeed, thanks. New diff below.
>
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/mail/exim/Makefile,v
> retrieving revision 1.136
> diff -u -p -r1.136 Makefile
> --- Makefile 5 May 2021 15:08:15 -0000 1.136
> +++ Makefile 2 Sep 2021 20:43:34 -0000
> @@ -8,7 +8,7 @@ DISTNAME = exim-${VERSION}
> PKGNAME-main = exim-${VERSION}
> FULLPKGNAME-eximon = exim-eximon-${VERSION}
> FULLPKGPATH-eximon = ${PKGPATH},-eximon
> -REVISION-main = 1
> +REVISION-main = 2
>
> CATEGORIES = mail
>
> Index: patches/patch-src_acl_c
> ===================================================================
> RCS file: patches/patch-src_acl_c
> diff -N patches/patch-src_acl_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_acl_c 2 Sep 2021 20:43:34 -0000
> @@ -0,0 +1,23 @@
> +$OpenBSD$
> +
> +Don't use printf %n.
> +
> +Index: src/acl.c
> +--- src/acl.c.orig
> ++++ src/acl.c
> +@@ -2906,10 +2906,12 @@ for (; cb; cb = cb->next)
> +
> + HDEBUG(D_acl)
> + {
> +- int lhswidth = 0;
> +- debug_printf_indent("check %s%s %n",
> ++ uschar buf[256];
> ++ int lhswidth = snprintf(CS buf, sizeof buf, "check %s%s ",
> + (!conditions[cb->type].is_modifier && cb->u.negated)? "!":"",
> +- conditions[cb->type].name, &lhswidth);
> ++ conditions[cb->type].name);
> ++ if (lhswidth == -1) lhswidth = 0;
> ++ debug_printf_indent("%s", buf);
> +
> + if (cb->type == ACLC_SET)
> + {
> Index: patches/patch-src_transport_c
> ===================================================================
> RCS file: patches/patch-src_transport_c
> diff -N patches/patch-src_transport_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_transport_c 2 Sep 2021 20:43:34 -0000
> @@ -0,0 +1,20 @@
> +$OpenBSD$
> +
> +Don't use printf %n.
> +
> +Index: src/transport.c
> +--- src/transport.c.orig
> ++++ src/transport.c
> +@@ -958,10 +958,9 @@ if (!(tctx->options & topt_no_headers))
> +
> + if (tctx->options & topt_add_return_path)
> + {
> +- int n;
> + uschar * s = string_sprintf("Return-path: <%.*s>\n%n",
> +- EXIM_EMAILADDR_MAX, return_path, &n);
> +- if (!write_chunk(tctx, s, n)) goto bad;
> ++ EXIM_EMAILADDR_MAX, return_path);
> ++ if (!write_chunk(tctx, s, strlen(s))) goto bad;
> + }
> +
> + /* Add envelope-to: if requested */
>
>

I discussed with exim guys and it seems they are quiet reluctant at
modifying "correct C code". At least the acl.c one will cause issues as
we have seen with the report from naddy@. So I propose to already commit
that diff and check further if there are other issues.