Error in getaddrinfo()?

Error in getaddrinfo()?

[Markus Wichmann]

Hi all,

I was just reading the getaddrinfo() source code, and I noticed
something funny. On line 107 we have the wonderful text:

		out[k].slot = i;

In context, k counts through all the outputs, i counts all the addresses
and j counts the ports. I was wondering about this line and looked for
where slot might be used. Its only use is in freeaddrinfo(), where it is
used to find the head of the output array. But then the slot should be
set to k, right?

I mean, it works wonderfully in the normal use case, where you call
freeaddrinfo() with the first element of the list. It also works in all
cases if nservs == 1, which is the case if the IPPROTO is set explicitly
in the hints structure. But otherwise?

According to POSIX, freeaddrinfo() has to support freeing "arbitrary
sublists" of the list originally returned by getaddrinfo(). I presume
that means "tail", because there is no way to free only a certain middle
part of the list.

And while we're on the subject, a few lines later we get

			.ai_next = &out[k+1].ai };

Now, for the last k, isn't this calculation undefined? The array index
is out of bounds, then. It won't matter what is calculated here, since
the last .ai_next is explicitly nulled a few lines further down, but the
calculation might invoke undefined behavior, and these last few years
compilers have gotten really agressive about that.

Ciao,
Markus

Re: Error in getaddrinfo()?

[Michael Forney]

On 2019-02-19, Markus Wichmann <nullplan@gmx.net> wrote:
> And while we're on the subject, a few lines later we get
>
> 			.ai_next = &out[k+1].ai };
>
> Now, for the last k, isn't this calculation undefined? The array index
> is out of bounds, then. It won't matter what is calculated here, since
> the last .ai_next is explicitly nulled a few lines further down, but the
> calculation might invoke undefined behavior, and these last few years
> compilers have gotten really agressive about that.

I don't think it is undefined behavior, as long as it is not
dereferenced. See http://port70.net/~nsz/c/c11/n1570.html#6.5.6p8:

"If both the pointer operand and the result point to elements of the
same array object, or one past the last element of the array object,
the evaluation shall not produce an overflow; otherwise, the behavior
is undefined. If the result points one past the last element of the
array object, it shall not be used as the operand of a unary *
operator that is evaluated."

Re: Error in getaddrinfo()?

[Rich Felker]

On Tue, Feb 19, 2019 at 01:09:21PM -0800, Michael Forney wrote:
> On 2019-02-19, Markus Wichmann <nullplan@gmx.net> wrote:
> > And while we're on the subject, a few lines later we get
> >
> > 			.ai_next = &out[k+1].ai };
> >
> > Now, for the last k, isn't this calculation undefined? The array index
> > is out of bounds, then. It won't matter what is calculated here, since
> > the last .ai_next is explicitly nulled a few lines further down, but the
> > calculation might invoke undefined behavior, and these last few years
> > compilers have gotten really agressive about that.
> 
> I don't think it is undefined behavior, as long as it is not
> dereferenced. See http://port70.net/~nsz/c/c11/n1570.html#6.5.6p8:
> 
> "If both the pointer operand and the result point to elements of the
> same array object, or one past the last element of the array object,
> the evaluation shall not produce an overflow; otherwise, the behavior
> is undefined. If the result points one past the last element of the
> array object, it shall not be used as the operand of a unary *
> operator that is evaluated."

This would be true if not for the ".ai". As written, I think it may be
UB, but it's questionable whether that depends on how much extra space
was allocated for ai_canonname. In any case it's bad. The initializer
should probably be 0, followed by:

	if (k) out[k-1].ai.ai_next = &out[k].ai;

That would get rid of the need to later zero the final one, too.

Rich

Re: Error in getaddrinfo()?

[Rich Felker]

On Tue, Feb 19, 2019 at 09:27:00PM +0100, Markus Wichmann wrote:
> Hi all,
> 
> I was just reading the getaddrinfo() source code, and I noticed
> something funny. On line 107 we have the wonderful text:
> 
> 		out[k].slot = i;
> 
> In context, k counts through all the outputs, i counts all the addresses
> and j counts the ports. I was wondering about this line and looked for
> where slot might be used. Its only use is in freeaddrinfo(), where it is
> used to find the head of the output array. But then the slot should be
> set to k, right?
> 
> I mean, it works wonderfully in the normal use case, where you call
> freeaddrinfo() with the first element of the list. It also works in all
> cases if nservs == 1, which is the case if the IPPROTO is set explicitly
> in the hints structure. But otherwise?
> 
> According to POSIX, freeaddrinfo() has to support freeing "arbitrary
> sublists" of the list originally returned by getaddrinfo(). I presume
> that means "tail", because there is no way to free only a certain middle
> part of the list.

I think you're right, and this should be k, not i. From your reading,
will that fix the problem?

Rich

Re: Error in getaddrinfo()?

[Markus Wichmann]

On Tue, Feb 19, 2019 at 05:31:45PM -0500, Rich Felker wrote:
> I think you're right, and this should be k, not i. From your reading,
> will that fix the problem?
> 
> Rich

Yes it will. I was also unable to find any new problem that it would
add, so that's always positive.

Ciao,
Markus