LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: daw@cs.berkeley.edu (David Wagner)
To: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy
Date: Fri, 7 Mar 2008 01:51:54 +0000 (UTC)	[thread overview]
Message-ID: <fqq73q$21t$1@taverner.cs.berkeley.edu> (raw)
In-Reply-To: <47D07FDC.2010701@tiscali.nl>

Roel Kluin  wrote:
>As I understand it, please correct me if I'm wrong:
>
>Of the three variants: strcpy, strncpy and strlcpy.
>- strcpy does not append \0 (unless the source string already contained it)

No, that's not correct.  If the source string has no \0, then
strcpy keeps reading forever and things go horribly awry.

>In the original code strncpy was used and the size parameter was equal
>to the source string size:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
>
>Since this the size was equal there was no \0 termination. To \0
>terminate using strncpy we could write:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id) - 1);
>dev->dev.bus_id[sizeof(dev->dev.bus_id) - 1] = '\0';
>
>or using strlcpy, which does the same thing:
>
>strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));

No, strlcpy() does not do the same thing.  As several people have pointed
out to you, strlcpy() does not fill the rest of the buffer with \0's.
This can create an information leak in some cases, which can create a
security hole.  The difference is important in some cases.

Second, you seem to implicitly assume that it's a bug to leave
dev->dev.bus_id without any \0 termination.  You'd have to look at the API
to determine whether that assumption is accurate.  And then you'd have to
think carefully about what is the proper behavior if info->name is too
long to fit into the buffer.  Is truncating the string the appropriate
behavior?  In some cases prematurely truncating a string is bad news and
can create a security bug, and the proper thing to do in some cases may
be to return an error rather than returning a truncated string.

Rant: Blindly search-and-replacing strncpy() with strlcpy() is not safe,
in general.  You have to actually understand what the code is doing and
what it should be doing.

I'm not claiming that your patch is correct, nor that it is incorrect.
Instead, I'm suggesting that you probably need to do some analysis to
understand the specific requirements and context in each of these cases,
rather than doing a mechanically search-and-replace.

      reply	other threads:[~2008-03-07  2:50 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-06 13:08 Roel Kluin
2008-03-06 21:44 ` H. Peter Anvin
2008-03-06 23:35   ` Roel Kluin
2008-03-07  1:51     ` David Wagner [this message]

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='fqq73q$21t$1@taverner.cs.berkeley.edu' \
    --to=daw@cs.berkeley.edu \
    --cc=daw-usenet@taverner.cs.berkeley.edu \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH] locomo.c: convert strncpy(x, y, sizeof(x)) to strlcpy' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).