Subject: Re: Revised address selection preference API draft-chakrabarti-ipv6-addrselect-api-04

From: "Samita Chakrabarti" <[email-masked]>
Date: Thu, 9 Nov 2006 19:45:29 -0800

Hello Rémi,

Thanks for your time reviewing V04 of the address selection API.
Julien L. has already addressed your comments. Here are some
additional comments.

The proposed solution of extending the getaddrinfo structure (at the end)
is designed to keep ABI consistency in mind.
The extended ai_eflags will not break ABI boundaries unless the library
design is broken. Usually libraries maintain versions for binary compatibility
reasons and new programs with newer data structure should be compiled
for the new library version. The old application will run fine with
the new library
as they have not used the extended flags. On the otherhand, the old
library should not recognize AI_PREFERENCES as a valid flag.

Thus, your concern on ABI compatibility is not a practical one. Thanks for the
thorough reviewing though.

Regards,
-Samita

> > One goal of this API is to be preserve the existing
> > basic IPv6 socket API.
>
> Expanding a structure breaks the API already.
>
> > Besides, I don't understand how would ai_connaddr and
> > ai_bindaddr solve the issue you describe.
>
> It makes sure the source address that will actually be bound in the one
> that was selected by getaddrinfo().
>
> > It should never be assumed (by application, library
> > kernel...) that the ai_eflags field is present in a
> > struct addrinfo if the struct doesn't have the
> > AI_PREFERENCES flag set in its ai_flags field.
>
> IMHO, that's an ugly hack. It breaks the ABI.
>
> > Hence a buffer overflow shouldn't happen unless the
> > library of you scenario is broken.
>
> Sure, but that does not solve my problem.
>
> In fact buffer overflow will happen in even less obvious ways. If some
> function compiled against the new ABI memset() an addrinfo pointer to a
> structure allocated by a function built with the old ABI, you have a
> buffer overflow, and well, you can't test for AI_PREFERENCES, since you
> don't even know it exists. Very bad idea. Changing a struct storage
> size is always a very bad idea. Why do you think people invented struct
> sockaddr_storage instead of increasing the size of struct sockaddr when
> rolling out the IPv6 API?
>
> > > IMHO, it might be wiser to define extended
> > > getaddrinfo, freeaddrinfo and struct addrinfo
> > > variants (geteaddrinfo ?) - that woud allow to use
> > > the ai_bindaddr and ai_connaddr thing at the same
> > > time.
>
> > IMHO it's nice to avoid introducing yet another name
> > resolution primitive. Regarding ai_bindaddr and
> > ai_connaddr, see my comment above.
>
> I don't see any point in sparing extra primitive and structure names.
> I think that it is much nicer than partially breaking the ABI by
> changing the size of structure.
>
> --
> Rémi Denis-Courmont
>
>
>

--------------------------------------------------------------------
IETF IPv6 working group mailing list
[email-masked]
Administrative Requests: https://www1.ietf.org/mailman/listinfo/ipv6
--------------------------------------------------------------------

---

From: "Samita Chakrabarti" <[email-masked]>
Date: Thu, 9 Nov 2006 20:11:52 -0800

> This is all a matter of being consistent with the Advanced IPv6 API, and
> with the established practice in POSIX world. Pretty much every
> setsockopt uses int, unless it has very peculiar reason not to do so.
> Using something else will only confuse people, and add a (minor) extra
> implementation burden on implementors.

Please note that this API document is a guideline to the community for
developing address selection implementation that wants to alter the default.
IETF does not specify  formal API specification, this API
is intended to be used as a guideline for POSIX world, if they decide to define
such API someday.

uint32_t is used for examples and used as type of flags for inet6_is_srcaddr().
It makes more sense for a flag to be an unsigned quantity. This document
has gone through experts review and no one objected to this.
But, changing uint32_t to int is not a big deal if  working group
feels that flags
should be type 'int' for consistency.

Does anyone else in the working group feel strongly about it?

>
> I do think it is semantically clean to use an "impossible" value as
> restore to system default, while it is NOT clean to use a "possible"
> value for that.
>

Point taken. The authors will discuss this in the light of practical
deployment  applications.

>
> Have you had a look at RFC3542 at all? (hint: §6.3 and §6.5)

Yes, we did. In fact if  you look at it carefully, you can see an
overlap of  a co-author in both documents :-)

Regards,
-Samita

---

From: Rémi Denis-Courmont <[email-masked]>
Date: Fri, 10 Nov 2006 09:07:56 +0200

Hello,

> library should not recognize AI_PREFERENCES as a valid flag.

I cannot agree with you. Changing "sizeof (struct addrinfo)" will break
code that was properly designed against the old API.

Just imagine you have some library function that looks like:

int my_getaddrinfo (const char *server, const struct addrinfo *hints,
...)
{
	struct addrinfo modified_hints;
	/* ... */
	memcpy (&modified_hints, hints, sizeof (hints));
	modified_hints.ai_flags |= AI_V4MAPPED;
	/* ... */
}

Now imagine that this function is compiled against a newer C library
that would include your API *and ABI* change, but the caller is not
recompiled. The hints structure will be sized to the old struct
addrinfo, but the memcpy will try to copy it as if it had the new bigger
size, and well, that's of course pretty.

We might not have the same definition of ABI compatibility, but to me,
changing the size of a public structure is an incompatible ABI change.

> Thus, your concern on ABI compatibility is not a practical one.

I know code that is correct nowadays and will become incorrect with the
proposed changes.

I'd like to have external opinions on this... maybe I'm wrong afterwall.
glibc anyone?

-- 
Rémi Denis-Courmont