From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12812 Path: news.gmane.org!.POSTED!not-for-mail From: Will Dietz Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] iconv: add check to avoid writing past end of buffer Date: Mon, 7 May 2018 16:08:33 -0500 Message-ID: References: <20180507192821.GL1392@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" X-Trace: blaine.gmane.org 1525727204 23935 195.159.176.226 (7 May 2018 21:06:44 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 7 May 2018 21:06:44 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-12828-gllmg-musl=m.gmane.org@lists.openwall.com Mon May 07 23:06:40 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1fFnLC-00066b-Rd for gllmg-musl@m.gmane.org; Mon, 07 May 2018 23:06:39 +0200 Original-Received: (qmail 1434 invoked by uid 550); 7 May 2018 21:08:46 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 1410 invoked from network); 7 May 2018 21:08:45 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wdtz.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=JxTsBtrRsZ1qmZesxfsykWv2m9kR38yZPqRpLZGraDQ=; b=LtGtSrya5dEfGcJW2L8m4W7dJk/mCcrmBC7Z2XGZEPoNHWM1Wbz88w3Nc1PULf2pJw 3X52g4oBxdV6Iy+BWn8FScIaqKH67uxHfiYg5VSB7qndq50DXsFVbwZGbf5Nh5p3igSO jaBAaNRBmSOgRAfTE2R9/4FhgHtJzanafkf+4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=JxTsBtrRsZ1qmZesxfsykWv2m9kR38yZPqRpLZGraDQ=; b=VtGdj1nVWsL7RWFRSB3cXR1EAUIbZiK2ErmYCp3CCwK+/e9Wl7VVH7ZJO0Lms70Ik6 ITyvc/oW5fIEbK0sm3flmaNOvsCFq2sfI27dL7zH1ReubF8QVXCSbLEaisgHExPNzTIv YA2iUVfJD5V+jdLxE/kQ4hHiUTyX5qoOwDKHa8ycl7+FSuAEtlPldSG3BZuGaG3okw4q iT5QxoyKIqODcfFn/ydzhhGK178fTN4Y9EpKBZrKgGbnjICEOtSJco71l5/pero+aElh w92ELFt1oqTJWcVLCIt3W/VrhMEW2PtgO5nVwCZuZli/eb8VQokSB/Q8qdih0TiOxqG3 QRzQ== X-Gm-Message-State: ALKqPwexwZ4wuY24VXfKbxZ96lnBQY93Nm2jF+Jg+rTKzBZ1zo2hld9u p+wiWDsDVeGzdIfFXH94GMDa5YNKE20ieJ2cvAAjVmM= X-Google-Smtp-Source: AB8JxZoNONPzQbvgUGRdj/dDcp0lqNOOdeRtBZ1XHdt0snJwkiLAtojYad105JffL5je4bOLSxKBIIg/lRm8Nl5xEWM= X-Received: by 2002:a9d:2842:: with SMTP id h2-v6mr8989219otd.210.1525727313855; Mon, 07 May 2018 14:08:33 -0700 (PDT) X-Originating-IP: [130.126.255.172] In-Reply-To: <20180507192821.GL1392@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:12812 Archived-At: On Mon, May 7, 2018 at 2:28 PM, Rich Felker wrote: > On Wed, May 02, 2018 at 12:07:17PM -0500, Will Dietz wrote: >> Attached. >> >> Example based on [1] that crashes without this fix can be found here: >> >> https://gist.github.com/7bc07da1dcd02e01c2fbb28cbaa81420 >> >> Input is from git's tests (2.17.0), and fixes tests when using >> noxcuse-based iconv utility and musl's iconv implementation. >> >> Well, *almost*. At least no more crashing :). One final test >> involving autosquash fails-- I believe due to a comparison breaking >> due to unexpected shifts in ISO-2022-JP encoding (as described in [2]) >> but I'm not sure of details just yet. Neat to get this far! >> >> ~Will >> >> [1] http://www.openwall.com/lists/musl/2017/05/03/1 >> [2] http://www.openwall.com/lists/musl/2014/11/09/1 > >> From d4516bbca6b315927b82252baa24574ae12f0b06 Mon Sep 17 00:00:00 2001 >> From: Will Dietz >> Date: Tue, 1 May 2018 14:16:44 -0500 >> Subject: [PATCH] iconv.c: add missing check against output buffer size >> >> --- >> src/locale/iconv.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/locale/iconv.c b/src/locale/iconv.c >> index d469856c..3c1f4dd2 100644 >> --- a/src/locale/iconv.c >> +++ b/src/locale/iconv.c >> @@ -539,6 +539,7 @@ size_t iconv(iconv_t cd, char **restrict in, size_t *restrict inb, char **restri >> if (*outb < 1) goto toobig; >> if (c<256 && c==legacy_map(tomap, c)) { >> revout: >> + if (*outb < 1) goto toobig; >> *(*out)++ = c; >> *outb -= 1; >> break; >> -- >> 2.17.0 >> > > This also looks correct. I'm not too fond of the location of the > check but I'm not sure there's anywhere I'd like much better. The > other options look like at the beginning of each relevant case or > before the whole switch. In some ways before the switch is nice > (eliminates lots of duplicate checks) but it's redundant in the case > of dest encodings that are always >1 byte. > Agreed about the location of the check--particularly with a similar check made two lines earlier! I also was unhappy about it but ultimately left it this way for sake of simplicity. Presumably this goto pattern is for optimizing code size (only one copy of code between 'revout:' and 'break;')? I'm somewhat curious if today's compilers don't move/copy this fragment anyway, in which case the redundant checks will be easily discarded. Maybe. In favor of not hoisting the check out of the switch: keeping the checks near the output makes it very easy to verify the right bounds are checked (or in this case, aren't). Given how complex the control-flow is in this function, this seems rather valuable-- especially from the perspective of ensuring correctness across future changes. Just some thoughts, and of course defer to you for ruling on aesthetic preferences and what you find easiest to read/maintain :). ~Will > Rich