All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Jones <pjones@redhat.com>
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: linux-efi@vger.kernel.org, Jason Andryuk <jandryuk@gmail.com>,
	intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Matthew Garrett <mjg59@coreos.com>,
	Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8
Date: Thu, 21 Apr 2016 11:13:44 -0400	[thread overview]
Message-ID: <20160421151343.GA3763@redhat.com> (raw)
In-Reply-To: <20160421121827.GE2829@codeblueprint.co.uk>

On Thu, Apr 21, 2016 at 01:18:27PM +0100, Matt Fleming wrote:
> ( Good Lord, I hate doing string manipulation in C )

(yep)

> 
> On Wed, 20 Apr, at 03:25:32PM, Laszlo Ersek wrote:
> > 
> > So, "len" does not include the room for the terminating NUL-byte here.
> > When "len" is passed to ucs2_as_utf8(), with the proposed patch applied,
> > a NUL byte will be produced in "name", but it will be at the price of a
> > genuine character from the input variable name.
> 
> Right, and this is a problem because we're trying to keep the names
> consistent between efivarfs and the EFI variable data. Force
> NUL-terminating the string is wrong, because if you have no room for
> the NUL the caller should check for that. Sadly none do.
> 
> On the flip-side, passing around non-NUL terminated strings is just
> begging for these kinds of issues to come up.
> 
> The fact is that the callers of ucs2_as_utf8() are passing it the
> wrong 'len' argument. We want a NUL-terminated utf8 string and we're
> passing a NUL-terminated ucs2 string. We should tell ucs2_as_utf8() it
> has enough room to copy the NUL.
> 
> Wouldn't this work (minus the return value checking)?

I agree with your analysis, and your patch looks plausible.

-- 
  Peter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

WARNING: multiple messages have this Message-ID (diff)
From: Peter Jones <pjones@redhat.com>
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Laszlo Ersek <lersek@redhat.com>,
	Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org,
	Jason Andryuk <jandryuk@gmail.com>,
	Matthew Garrett <mjg59@coreos.com>,
	linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8
Date: Thu, 21 Apr 2016 11:13:44 -0400	[thread overview]
Message-ID: <20160421151343.GA3763@redhat.com> (raw)
In-Reply-To: <20160421121827.GE2829@codeblueprint.co.uk>

On Thu, Apr 21, 2016 at 01:18:27PM +0100, Matt Fleming wrote:
> ( Good Lord, I hate doing string manipulation in C )

(yep)

> 
> On Wed, 20 Apr, at 03:25:32PM, Laszlo Ersek wrote:
> > 
> > So, "len" does not include the room for the terminating NUL-byte here.
> > When "len" is passed to ucs2_as_utf8(), with the proposed patch applied,
> > a NUL byte will be produced in "name", but it will be at the price of a
> > genuine character from the input variable name.
> 
> Right, and this is a problem because we're trying to keep the names
> consistent between efivarfs and the EFI variable data. Force
> NUL-terminating the string is wrong, because if you have no room for
> the NUL the caller should check for that. Sadly none do.
> 
> On the flip-side, passing around non-NUL terminated strings is just
> begging for these kinds of issues to come up.
> 
> The fact is that the callers of ucs2_as_utf8() are passing it the
> wrong 'len' argument. We want a NUL-terminated utf8 string and we're
> passing a NUL-terminated ucs2 string. We should tell ucs2_as_utf8() it
> has enough room to copy the NUL.
> 
> Wouldn't this work (minus the return value checking)?

I agree with your analysis, and your patch looks plausible.

-- 
  Peter

  reply	other threads:[~2016-04-21 15:13 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-20  8:37 [PATCH] lib: Always NUL terminate ucs2_as_utf8 Chris Wilson
2016-04-20  8:37 ` Chris Wilson
     [not found] ` <1461141427-16361-1-git-send-email-chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
2016-04-20  9:36   ` Laszlo Ersek
2016-04-20  9:36     ` Laszlo Ersek
2016-04-20  9:41     ` Chris Wilson
2016-04-20  9:41       ` Chris Wilson
2016-04-20 12:45       ` Laszlo Ersek
2016-04-20 12:45         ` Laszlo Ersek
2016-04-20 13:25   ` Laszlo Ersek
2016-04-20 13:25     ` Laszlo Ersek
     [not found]     ` <5717834C.6070802-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-04-21 12:18       ` Matt Fleming
2016-04-21 12:18         ` Matt Fleming
2016-04-21 15:13         ` Peter Jones [this message]
2016-04-21 15:13           ` Peter Jones
2016-04-22 11:27           ` Dave Gordon
2016-04-21 16:21         ` Laszlo Ersek
2016-04-21 16:21           ` Laszlo Ersek
2016-04-22 18:52           ` Matt Fleming
2016-04-22 18:52             ` Matt Fleming
     [not found]             ` <20160422185210.GG2829-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
2016-04-25 10:17               ` Laszlo Ersek
2016-04-25 10:17                 ` Laszlo Ersek
2016-04-20 14:03   ` [Intel-gfx] " Jani Nikula
2016-04-20 14:03     ` Jani Nikula
2016-04-20 10:57 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-04-22  9:58 ` ✗ Fi.CI.BAT: warning " Patchwork
2016-04-22 16:27 ` ✓ Fi.CI.BAT: success " Patchwork
2016-04-24 13:36 ` Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160421151343.GA3763@redhat.com \
    --to=pjones@redhat.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=jandryuk@gmail.com \
    --cc=lersek@redhat.com \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matt@codeblueprint.co.uk \
    --cc=mjg59@coreos.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.