wild use of strlcpy() broke ipv6 support
Jeremy Allison
jra at samba.org
Fri Mar 30 21:52:09 MDT 2012
On Fri, Mar 30, 2012 at 06:02:06PM -0700, Matthieu Patou wrote:
> At least for this two changes you didn't get completely the sense of
> the strlcpy(), the idea is that if you have
> fe80::221:ccff:fe5f:7e51%eth0 to get the number of the interface and
> remove what is after '%'.
> so we should in this case always truncate and ihmo it's not a problem.
Ok - I'm boilerplate fixing all strlcpy uses. If it truncates
then by definition it's a problem, and we shouldn't be using
strlcpy() - we should be correctly paying attention to
the length we really want.
I do (vaguely :-) remember writing this code (or something
like it). I'll take a look and fix up asap.
> > * zero IPv6 address. No good choice here.
> > */
> >
> >- strlcpy(addr_buf, "0.0.0.0", addr_len);
> >+ if (strlcpy(addr_buf, "0.0.0.0", addr_len)>= addr_len) {
> >+ /* Truncate ! */
> >+ return NULL;
> >+ }
> Here it seems also wrong if you have an address like 1.2.3.4 as
> addr_len will be the same as strlen("0.0.0.0"), didn't had a closer
> look.
No this is correct. addr_len is passed in here - we should never
be truncating.
> I just focused on the IPv6 issues, maybe we should revert your patch
> while you had a closer look on the different use cases for strlcpy
> and then push a valid change ?
I don't think so. I already found one truncate issue with
these changes so they're the correct thing to do.
strlcpy/strlcat should not be used without truncate
checks. Doing so can court disaster.
Jeremy.
More information about the samba-technical
mailing list