From: Thomas Fjellstrom <thomas@fjellstrom.ca>
To: Jes Sorensen <Jes.Sorensen@redhat.com>
Cc: linux-raid@vger.kernel.org
Subject: Re: [PATCH] mdadm: replace hard coded string length
Date: Tue, 20 Sep 2016 12:03:18 -0600 [thread overview]
Message-ID: <1794768.TENJQ1DrVS@natasha> (raw)
In-Reply-To: <wrfjpoo4qazf.fsf@redhat.com>
On Friday, September 16, 2016 8:34:44 AM MDT Jes Sorensen wrote:
> Thomas Fjellstrom <thomas@fjellstrom.ca> writes:
> > On Thursday, September 15, 2016 12:15:30 PM MDT Jes Sorensen wrote:
> >> Song Liu <songliubraving@fb.com> writes:
> >> > @@ -1124,7 +1124,7 @@ static int update_super1(struct supertype *st,
> >> > struct mdinfo *info,>
> >> >
> >> > if (c)
> >> >
> >> > strncpy(info->name, c+1, 31 - (c-sb->set_name));
> >> >
> >> > else
> >> >
> >> > - strncpy(info->name, sb->set_name, 32);
> >> > + strncpy(info->name, sb->set_name, sizeof(sb->set_name));
> >> >
> >> > info->name[32] = 0;
> >> >
> >> > }
> >>
> >> I was about to apply this, but this is actually wrong. You need to use
> >> the size of the destination, not of the source as the limit.
> >>
> >> Sorry for the hassle.
> >
> > I'm not aware of the full details, but either they are the same size, or
> > they aren't, and you need to use the minimum size of both to avoid any
> > kind of overflow (source or dest, read and write). I presume the
> > destination is smaller?
>
> When copying a null terminated string, you need to check against the
> size of the destination, not the source. It may happen to be they are
> the same size here, but if code is later moved around you could get into
> a situation where that is no longer the case. Checking against the size
> of the destination is the correct way.
Yes, I wasn't paying close enough attention, str*cpy does the check on the
source length so yes, you want to check against just the destination length in
this case. In essense, you're checking both and clamping to the minimum of
either buffer.
> Second, when you reply to a mailing list posting, kindly refrain from
> removing the person you respond to from the CC list.
Sorry, I felt like it'd be spamming the two other people.
> Jes
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thomas Fjellstrom
thomas@fjellstrom.ca
prev parent reply other threads:[~2016-09-20 18:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-15 0:13 [PATCH] mdadm: replace hard coded string length Song Liu
2016-09-15 16:15 ` Jes Sorensen
2016-09-15 18:10 ` Thomas Fjellstrom
2016-09-15 23:34 ` Adam Goryachev
2016-09-16 12:38 ` Jes Sorensen
2016-09-16 12:34 ` Jes Sorensen
2016-09-20 18:03 ` Thomas Fjellstrom [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=1794768.TENJQ1DrVS@natasha \
--to=thomas@fjellstrom.ca \
--cc=Jes.Sorensen@redhat.com \
--cc=linux-raid@vger.kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox