Bugs in StrToHostAddr6 in sockets unit

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
Using fpc 3.0.4 on Fedora 30.

I've just started using the StrToHostAddr6 function in the sockets unit
to parse IPv6 addresses. I've found a couple of issues with it.

1. Even if address parsing fails, StrToHostAddr6 doesn't return an
all-zero result in the in6_addr return value. The documentation states
that it does, but my tests show it doesn't.

This can be trivially demonstrated with a program like so:

program ip6bug;

uses
   SysUtils, sockets;

procedure parse_print_ip6(addr: String);
var
   ip6: in6_addr;
   idx: Cardinal;
begin
   ip6 := StrToHostAddr6(addr);
   write('"' + addr+'" ' + '-> ');
   for idx := 0 to 15 do
     write(IntToHex(byte(ip6.s6_addr8[idx]),2)+'-');
   writeln();
end;

const
   addr = 'fxg%::906e:9d2f:a520:4172';
begin
   parse_print_ip6(addr);
end.

On my machine this produces:

"fxg%::906e:9d2f:a520:4172" ->
00-00-00-60-68-74-E0-00-00-00-00-61-4F-2B-60-00-

The problem is that the StrToHostAddr6 function doesn't set its return
value until the end of the function. If a parse error occurs
mid-function, it zeroes the record in which it's writing the result but
exits without setting the function return value. What gets returned
depends on what's on the stack.

I could have sworn that fpc would detect a function exiting without
setting a return value, but clearly 3.0.4 doesn't.

2. The second problem is that a colon-separated string containing
hexadecimal values of any length will be parsed and treated as valid by
StrToHostAddr6. E.g, it will parse a string like
"fe80ca2f1::906e:9d2f:a520:4172". The sample program produces this
output for this string:

"fe80ca2f1::906e:9d2f:a520:4172" ->
A2-F1-00-00-00-00-00-00-90-6E-9D-2F-A5-20-41-72-

For the part before the first colon, it has discarded all but the last
two bytes, A2 and F1. But it should not have accepted this string at
all. There can be only four characters between the colons.

Neither of these are difficult to fix. But I am curious to know why the
compiler lets a function return without setting a return value and
doesn't at least issue a warning. I know it does in some cases, because
I've often seen the message "Function result doesn't appear to be set".
But maybe that's only if you never set the result, as opposed to setting
it only for some paths through the code.
_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Michael Van Canneyt


On Sun, 3 May 2020, Noel Duffy via fpc-pascal wrote:

> Using fpc 3.0.4 on Fedora 30.
>
> I've just started using the StrToHostAddr6 function in the sockets unit
> to parse IPv6 addresses. I've found a couple of issues with it.
>
> 1. Even if address parsing fails, StrToHostAddr6 doesn't return an
> all-zero result in the in6_addr return value. The documentation states
> that it does, but my tests show it doesn't.

I fixed that.

>
> The problem is that the StrToHostAddr6 function doesn't set its return
> value until the end of the function. If a parse error occurs
> mid-function, it zeroes the record in which it's writing the result but
> exits without setting the function return value. What gets returned
> depends on what's on the stack.
>
> I could have sworn that fpc would detect a function exiting without
> setting a return value, but clearly 3.0.4 doesn't.

It checks if the result is assigned. This is done;
But it does not check this for every exit, as far as I know it never has.

>
> 2. The second problem is that a colon-separated string containing
> hexadecimal values of any length will be parsed and treated as valid by
> StrToHostAddr6. E.g, it will parse a string like
> "fe80ca2f1::906e:9d2f:a520:4172". The sample program produces this
> output for this string:
>
> "fe80ca2f1::906e:9d2f:a520:4172" ->
> A2-F1-00-00-00-00-00-00-90-6E-9D-2F-A5-20-41-72-
>
> For the part before the first colon, it has discarded all but the last
> two bytes, A2 and F1. But it should not have accepted this string at
> all. There can be only four characters between the colons.

If you send a patch for this, I'll apply it.

>
> Neither of these are difficult to fix. But I am curious to know why the
> compiler lets a function return without setting a return value and
> doesn't at least issue a warning. I know it does in some cases, because
> I've often seen the message "Function result doesn't appear to be set".
> But maybe that's only if you never set the result, as opposed to setting
> it only for some paths through the code.

Exactly.

Michael.
_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
In reply to this post by Free Pascal - General mailing list
On Sun, 3 May 2020 09:57:46 +0200 (CEST) Michael Van Canneyt
<[hidden email]> wrote:

>
> On Sun, 3 May 2020, Noel Duffy via fpc-pascal wrote:
>>
>> The problem is that the StrToHostAddr6 function doesn't set its return
>> value until the end of the function. If a parse error occurs
>> mid-function, it zeroes the record in which it's writing the result but
>> exits without setting the function return value. What gets returned
>> depends on what's on the stack.
>>
>> I could have sworn that fpc would detect a function exiting without
>> setting a return value, but clearly 3.0.4 doesn't.
>
> It checks if the result is assigned. This is done;
> But it does not check this for every exit, as far as I know it never has.

That seems like it would be a valuable feature to have. I am not certain
what's contained in an uninitialized variable that gets returned from a
function but I'd guess there's a risk of information leakage.

The way return values from functions get set is one part of the Pascal
language I dislike.

>> 2. The second problem is that a colon-separated string containing
>> hexadecimal values of any length will be parsed and treated as valid by
>> StrToHostAddr6. E.g, it will parse a string like
>> "fe80ca2f1::906e:9d2f:a520:4172". The sample program produces this
>> output for this string:
>>
>> "fe80ca2f1::906e:9d2f:a520:4172" ->
>> A2-F1-00-00-00-00-00-00-90-6E-9D-2F-A5-20-41-72-
>>
>> For the part before the first colon, it has discarded all but the last
>> two bytes, A2 and F1. But it should not have accepted this string at
>> all. There can be only four characters between the colons.
>
> If you send a patch for this, I'll apply it.

Sure I can take a stab at this. Do you normally get people to open a bug
  against which to post the patch? I'm not at all familiar with
contributing to fpc, I'm afraid, so please bear with me! Also, if
there's information on unit testing for library functions and
procedures, that would be very helpful.
_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Michael Van Canneyt


On Sun, 3 May 2020, Noel Duffy via fpc-pascal wrote:

> On Sun, 3 May 2020 09:57:46 +0200 (CEST) Michael Van Canneyt
> <[hidden email]> wrote:
>>
>> On Sun, 3 May 2020, Noel Duffy via fpc-pascal wrote:
>>>
>>> The problem is that the StrToHostAddr6 function doesn't set its return
>>> value until the end of the function. If a parse error occurs
>>> mid-function, it zeroes the record in which it's writing the result but
>>> exits without setting the function return value. What gets returned
>>> depends on what's on the stack.
>>>
>>> I could have sworn that fpc would detect a function exiting without
>>> setting a return value, but clearly 3.0.4 doesn't.
>>
>> It checks if the result is assigned. This is done;
>> But it does not check this for every exit, as far as I know it never has.
>
> That seems like it would be a valuable feature to have. I am not certain
> what's contained in an uninitialized variable that gets returned from a
> function but I'd guess there's a risk of information leakage.

That's why the compiler warns about use of uninitialized variables.
But there will always be corner cases the compiler does not catch.

>>> For the part before the first colon, it has discarded all but the last
>>> two bytes, A2 and F1. But it should not have accepted this string at
>>> all. There can be only four characters between the colons.
>>
>> If you send a patch for this, I'll apply it.
>
> Sure I can take a stab at this. Do you normally get people to open a bug
>  against which to post the patch? I'm not at all familiar with
> contributing to fpc, I'm afraid, so please bear with me! Also, if
> there's information on unit testing for library functions and
> procedures, that would be very helpful.

Yes, please open a bug report. If you attach a small console test program that
demonstrates the bug (and subsequently the fix) then I will make sure it
ends up in the correct place. If you make sure it exits with exit code 0 if all is
well, and a nonzero exit code if there is an error, that will save me some
work. If you post the URL for the bug here, I will look at it at once.

Thanks for pointing it out!

Michael.
_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
On 3/05/20 10:28 pm, Michael Van Canneyt wrote:
>
>
> Yes, please open a bug report. If you attach a small console test
> program that
> demonstrates the bug (and subsequently the fix) then I will make sure it
> ends up in the correct place. If you make sure it exits with exit code 0
> if all is
> well, and a nonzero exit code if there is an error, that will save me some
> work. If you post the URL for the bug here, I will look at it at once.

Done. Bug #37013.

https://bugs.freepascal.org/view.php?id=37013

_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
In reply to this post by Michael Van Canneyt
On 3/05/20 10:28 pm, Michael Van Canneyt wrote:

>
> On Sun, 3 May 2020, Noel Duffy via fpc-pascal wrote:
>
>> On Sun, 3 May 2020 09:57:46 +0200 (CEST) Michael Van Canneyt
>> <[hidden email]> wrote:
>>>
> Yes, please open a bug report. If you attach a small console test
> program that
> demonstrates the bug (and subsequently the fix) then I will make sure it
> ends up in the correct place. If you make sure it exits with exit code 0
> if all is
> well, and a nonzero exit code if there is an error, that will save me some
> work. If you post the URL for the bug here, I will look at it at once.

To follow-up on this issue I raised in bug #37013
(https://bugs.freepascal.org/view.php?id=37013), I've been digging into
RFC4291 to see what address formats for IPv6 exist, and I see that the
scope of this problem is bigger than I envisioned.

To summarize, the current function, StrToHostAddr6, from the sockets
unit, will successfully parse many address formats expressly forbidden
by RFC4291, and it fails to parse others that the RFC expressly
requires. An example of the latter is this ugly form:

fe80:caf1:906e:9d2f:a520:4172:172.16.31.14

This apparently exists to make IPv4 to IPv6 transitions easier.

Anyway, back to the bug report. I can't make StrToHostAddr6 RFC4291
compliant without rewriting it, and I don't want to undertake that
unless I know it's something the FPC community actually wants. I am
happy to do the work, don't get me wrong, but I realise that a wholesale
  rewrite of the function is much wider in scope and has more potential
for drawbacks.

Testing would also have to be fairly rigorous to make sure all the
different formats are handled correctly. I've already done some  work to
make it easy to compare StrToHostAddr6's output to that of inet_pton in
the C library, so this would at least provide a good method to quickly
spot problems.

So I guess the question is, is it worth the effort to make
StrToHostAddr6 RFC4291 compliant? Is that something the FPC team would
want, or do they just not use the sockets unit?

_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
On Wed, May 6, 2020 at 12:19 PM Noel Duffy via fpc-pascal <[hidden email]> wrote:
So I guess the question is, is it worth the effort to make
StrToHostAddr6 RFC4291 compliant? Is that something the FPC team would
want, or do they just not use the sockets unit?

There have been 2 changes applied to sockets.inc in the last two months so it is not neglected.  
I am convinced a patch to ensure compliance will be considered for inclusion, especially if you 
provide test cases demonstrating shortcomings in the current implementation.


_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Karoly Balogh (Charlie/SGR)
In reply to this post by Free Pascal - General mailing list
Hi,

On Wed, 6 May 2020, Noel Duffy via fpc-pascal wrote:

> Testing would also have to be fairly rigorous to make sure all the
> different formats are handled correctly. I've already done some  work to
> make it easy to compare StrToHostAddr6's output to that of inet_pton in
> the C library, so this would at least provide a good method to quickly
> spot problems.
>
> So I guess the question is, is it worth the effort to make
> StrToHostAddr6 RFC4291 compliant? Is that something the FPC team would
> want,

Definitely yes. FPC is not a commercial "product", it's an open source
project, and contributions like this are the whole idea behind open
source, and makes projects like FPC actually viable. Note that the code
you're looking into is very old (almost 15 years old), and was a
contributed code in the first place, not written by the FPC team (see SVN
r939). And this also means it actually predates the RFC you refer to. :)
So it definitely needs an overhaul.

> or do they just not use the sockets unit?

As much as we use the compiler and associated libraries ourselves, we
can't cover every use case, or find every bug. A lot of packages are there
for completeness, or because someone submitted it, or for compatibility
with Delphi, not because we necessarily use it ourselves. So if you find
weak spots like this where you feel you can do better, do it and submit
it!

Charlie
_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
In reply to this post by Free Pascal - General mailing list
On 6/05/20 11:21 pm, Christo Crause via fpc-pascal wrote:
 > On Wed, May 6, 2020 at 12:19 PM Noel Duffy via fpc-pascal
 > <[hidden email]
 > <mailto:[hidden email]>>
 > wrote:
 >
 >     So I guess the question is, is it worth the effort to make
 >     StrToHostAddr6 RFC4291 compliant? Is that something the FPC team
would
 >     want, or do they just not use the sockets unit?
 >
 >
 > There have been 2 changes applied to sockets.inc in the last two
 > months so it is not neglected. I am convinced a patch to ensure
 > compliance will be considered for inclusion, especially if you
 > provide test cases demonstrating shortcomings in the current
 > implementation.

Great. My worry was that the sockets unit might fall into the same
category as the libc one, available but deprecated and not recommended
for use. Plus, when rewriting something that's been unchanged for years,
there's a risk of breaking programs that depend on the bug.

_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Michael Van Canneyt


On Thu, 7 May 2020, Noel Duffy via fpc-pascal wrote:

> On 6/05/20 11:21 pm, Christo Crause via fpc-pascal wrote:
> > On Wed, May 6, 2020 at 12:19 PM Noel Duffy via fpc-pascal
> > <[hidden email]
> > <mailto:[hidden email]>>
> > wrote:
> >
> >     So I guess the question is, is it worth the effort to make
> >     StrToHostAddr6 RFC4291 compliant? Is that something the FPC team
> would
> >     want, or do they just not use the sockets unit?
> >
> >
> > There have been 2 changes applied to sockets.inc in the last two
> > months so it is not neglected. I am convinced a patch to ensure
> > compliance will be considered for inclusion, especially if you
> > provide test cases demonstrating shortcomings in the current
> > implementation.
>
> Great. My worry was that the sockets unit might fall into the same
> category as the libc one, available but deprecated and not recommended
> for use. Plus, when rewriting something that's been unchanged for years,
> there's a risk of breaking programs that depend on the bug.

I think the processing of wrong IP addresses and missing IPv4-ending
addresses support is not really something one depends on :)
So fixing this is quite OK.

Michael.
_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
In reply to this post by Karoly Balogh (Charlie/SGR)
On 6/05/20 11:40 pm, Karoly Balogh (Charlie/SGR) wrote:
>
> As much as we use the compiler and associated libraries ourselves, we
> can't cover every use case, or find every bug. A lot of packages are there
> for completeness, or because someone submitted it, or for compatibility
> with Delphi, not because we necessarily use it ourselves. So if you find
> weak spots like this where you feel you can do better, do it and submit
> it!

Sure, I do understand this, but it can be difficult sometimes  to tell
whether a unit is actively used or whether it's a relic of some long
abandoned idea, like the libc unit for instance. Added to that, I am
aware that rewriting functions in old units risks breaking things, even
if the fix is correct, and so I feel it's prudent (and, hopefully,
polite) to ask before writing the code.


_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
In reply to this post by Michael Van Canneyt
On 7/05/20 9:17 pm, Michael Van Canneyt wrote:

>
> On Thu, 7 May 2020, Noel Duffy via fpc-pascal wrote:
>
>> Great. My worry was that the sockets unit might fall into the same
>> category as the libc one, available but deprecated and not recommended
>> for use. Plus, when rewriting something that's been unchanged for
>> years, there's a risk of breaking programs that depend on the bug.
>
> I think the processing of wrong IP addresses and missing IPv4-ending
> addresses support is not really something one depends on :)
> So fixing this is quite OK.

Heh! Guess I really am the only one who implements RFCs in Object Pascal
in his spare time. Maybe I should have learned to play golf :D



_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Michael Van Canneyt


On Thu, 7 May 2020, Noel Duffy via fpc-pascal wrote:

> On 7/05/20 9:17 pm, Michael Van Canneyt wrote:
>>
>> On Thu, 7 May 2020, Noel Duffy via fpc-pascal wrote:
>>
>>> Great. My worry was that the sockets unit might fall into the same
>>> category as the libc one, available but deprecated and not recommended
>>> for use. Plus, when rewriting something that's been unchanged for
>>> years, there's a risk of breaking programs that depend on the bug.
>>
>> I think the processing of wrong IP addresses and missing IPv4-ending
>> addresses support is not really something one depends on :)
>> So fixing this is quite OK.
>
> Heh! Guess I really am the only one who implements RFCs in Object Pascal
> in his spare time. Maybe I should have learned to play golf :D

Well, considering the amount of RFCs I implemented for FPC, I think I'll join you.
Let me know what golf course you picked. If your email address is any
indication, new zealand is a bit far off, but has always been on my bucket list ;-)

Michael.
_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
On 7/05/20 9:43 pm, Michael Van Canneyt wrote:
>
> On Thu, 7 May 2020, Noel Duffy via fpc-pascal wrote:
>
> Well, considering the amount of RFCs I implemented for FPC, I think
> I'll join you. Let me know what golf course you picked. If your email
> address is any indication, new zealand is a bit far off, but has
> always been on my bucket list ;-)

Yes, I'm in New Zealand. Right now there's a two-week quarantine for all
new arrivals though, and the golf courses are of course shut! Still,
plenty of time with nothing to do but write Pascal code and obsess over
  the minutiae of IETF standards!

_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal
Reply | Threaded
Open this post in threaded view
|

Re: Bugs in StrToHostAddr6 in sockets unit

Free Pascal - General mailing list
In reply to this post by Free Pascal - General mailing list
I've submitted a test program and proposed patch to fix the IPv6 parsing
issues with StrToHostAddr6.

https://bugs.freepascal.org/view.php?id=37013

Because I only checked out the packages and rtl directories from
Subversion, my patch's root directory is packages. I hope this isn't a
problem, but if it is I'll re-do it.



_______________________________________________
fpc-pascal maillist  -  [hidden email]
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-pascal