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-cvs mailing list