[v7,2/5] sockets: Change inet_parse() to accept address specification without port

sockets: Change inet_parse() to accept address specification without port

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

inet_parse() expects address:port. Change it to work without explicit port
specification. In addition, don't depend solely on the return value of
sscanf but also consider the value obtained for %n directive used in sscanf.
This ensures that the scanning of malformed inet address isn't flagged as
success.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---

 qemu-sockets.c |   32 +++++++++++++++++++++++++-------
 1 files changed, 25 insertions(+), 7 deletions(-)

Am 17.09.2012 17:23, schrieb Bharata B Rao:
> sockets: Change inet_parse() to accept address specification without port
> 
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> inet_parse() expects address:port. Change it to work without explicit port
> specification. In addition, don't depend solely on the return value of

Things like "in addition" in a commit message are almost always a sign
that the patch should be split in two.

> sscanf but also consider the value obtained for %n directive used in sscanf.
> This ensures that the scanning of malformed inet address isn't flagged as
> success.

Can you give an example string that would be falsely accepted? To me the
old checks look fine (even though the new ones are a little bit easier
to read, so even if they don't fix anything, they might be worth doing).

Anyway, it does look correct.

Kevin

Il 18/09/2012 15:22, Kevin Wolf ha scritto:
> Am 17.09.2012 17:23, schrieb Bharata B Rao:
>> sockets: Change inet_parse() to accept address specification without port
>>
>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>
>> inet_parse() expects address:port. Change it to work without explicit port
>> specification. In addition, don't depend solely on the return value of
> 
> Things like "in addition" in a commit message are almost always a sign
> that the patch should be split in two.
> 
>> sscanf but also consider the value obtained for %n directive used in sscanf.
>> This ensures that the scanning of malformed inet address isn't flagged as
>> success.
> 
> Can you give an example string that would be falsely accepted? To me the
> old checks look fine (even though the new ones are a little bit easier
> to read, so even if they don't fix anything, they might be worth doing).

"localhost" would fail to be parsed:

-        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
+        ret = sscanf(str, "%64[^:]%n:%32[^,]%n", addr, &addr_pos,
+            port, &port_pos);
+        if (addr_pos == -1 || ret == EOF) {

because the : in the format string would not match and sscanf would
return 1.

However, is it correct to set the port unconditionally to an empty
string?  Your usecase makes sense, but perhaps the default port be
passed as an extra parameter to inet_parse instead.

Paolo

> Anyway, it does look correct.
> 
> Kevin
> 
>

Am 18.09.2012 15:31, schrieb Paolo Bonzini:
> Il 18/09/2012 15:22, Kevin Wolf ha scritto:
>> Am 17.09.2012 17:23, schrieb Bharata B Rao:
>>> sockets: Change inet_parse() to accept address specification without port
>>>
>>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>>
>>> inet_parse() expects address:port. Change it to work without explicit port
>>> specification. In addition, don't depend solely on the return value of
>>
>> Things like "in addition" in a commit message are almost always a sign
>> that the patch should be split in two.
>>
>>> sscanf but also consider the value obtained for %n directive used in sscanf.
>>> This ensures that the scanning of malformed inet address isn't flagged as
>>> success.
>>
>> Can you give an example string that would be falsely accepted? To me the
>> old checks look fine (even though the new ones are a little bit easier
>> to read, so even if they don't fix anything, they might be worth doing).
> 
> "localhost" would fail to be parsed:
> 
> -        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
> +        ret = sscanf(str, "%64[^:]%n:%32[^,]%n", addr, &addr_pos,
> +            port, &port_pos);
> +        if (addr_pos == -1 || ret == EOF) {
> 
> because the : in the format string would not match and sscanf would
> return 1.

Yes, that's the part with making the port optional.

Bharata also claims that "scanning of malformed inet address" could
falsely succeed before, which I can't see (but which I suspect is what
the first two hunks of the patch are meant to address).

> However, is it correct to set the port unconditionally to an empty
> string?  Your usecase makes sense, but perhaps the default port be
> passed as an extra parameter to inet_parse instead.

I thought about this, too, but didn't care enough to mention it. Now
that we're two, yes, I'd like adding a default port parameter.

Kevin

On Tue, Sep 18, 2012 at 04:08:43PM +0200, Kevin Wolf wrote:
> Am 18.09.2012 15:31, schrieb Paolo Bonzini:
> > Il 18/09/2012 15:22, Kevin Wolf ha scritto:
> >> Am 17.09.2012 17:23, schrieb Bharata B Rao:
> >>> sockets: Change inet_parse() to accept address specification without port
> >>>
> >>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >>>
> >>> inet_parse() expects address:port. Change it to work without explicit port
> >>> specification. In addition, don't depend solely on the return value of
> >>
> >> Things like "in addition" in a commit message are almost always a sign
> >> that the patch should be split in two.

Both kind of go together. Not depending on return value of sscanf gives us
the ability to have the port as optional parameter. Will rephrase the patch
description accordingly.

> >>
> >>> sscanf but also consider the value obtained for %n directive used in sscanf.
> >>> This ensures that the scanning of malformed inet address isn't flagged as
> >>> success.
> >>
> >> Can you give an example string that would be falsely accepted? To me the
> >> old checks look fine (even though the new ones are a little bit easier
> >> to read, so even if they don't fix anything, they might be worth doing).
> > 
> > "localhost" would fail to be parsed:
> > 
> > -        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
> > +        ret = sscanf(str, "%64[^:]%n:%32[^,]%n", addr, &addr_pos,
> > +            port, &port_pos);
> > +        if (addr_pos == -1 || ret == EOF) {
> > 
> > because the : in the format string would not match and sscanf would
> > return 1.
> 
> Yes, that's the part with making the port optional.
> 
> Bharata also claims that "scanning of malformed inet address" could
> falsely succeed before, which I can't see (but which I suspect is what
> the first two hunks of the patch are meant to address).

For malformed ipv6 address like "[1:2:3:4:5", sccanf in inet_parse
returns 1 (which means 1 input item successfully matched and assigned)
The current inet_parse code would eventually fail it since it checks for
return value of 2, but when I am making port optional, I can't depend
on return value of 1 or 2 since sscanf can return 1 for such incomplete ipv6
addresses too. Note that in the above case, though sscanf returned 1, the
pos argument remains unchanged indicating that it couldn't really parse
any input correctly.

So in summary, when I said scanning of malformed inet address succeeded
earlier, I should have been more specific by saying that sscanf in inet_parse
could return success for malformed ipv6 strings.

> 
> > However, is it correct to set the port unconditionally to an empty
> > string?  Your usecase makes sense, but perhaps the default port be
> > passed as an extra parameter to inet_parse instead.
> 
> I thought about this, too, but didn't care enough to mention it. Now
> that we're two, yes, I'd like adding a default port parameter.

So you are saying that lets change inet_parse to look like this:

int inet_parse(QemuOpts *opts, const char *str, int port)

and if @str didn't specify a port explicitly, use @port to populate
the port option in @opts ? Other callers of inet_parse are inet_listen and
inet_connect. What should be the default port values from these callers ?

Regards,
Bharata.

Am 20.09.2012 08:30, schrieb Bharata B Rao:
> On Tue, Sep 18, 2012 at 04:08:43PM +0200, Kevin Wolf wrote:
>> Am 18.09.2012 15:31, schrieb Paolo Bonzini:
>>> Il 18/09/2012 15:22, Kevin Wolf ha scritto:
>>>> Am 17.09.2012 17:23, schrieb Bharata B Rao:
>>>>> sockets: Change inet_parse() to accept address specification without port
>>>>>
>>>>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>>>>
>>>>> inet_parse() expects address:port. Change it to work without explicit port
>>>>> specification. In addition, don't depend solely on the return value of
>>>>
>>>> Things like "in addition" in a commit message are almost always a sign
>>>> that the patch should be split in two.
> 
> Both kind of go together. Not depending on return value of sscanf gives us
> the ability to have the port as optional parameter. Will rephrase the patch
> description accordingly.
> 
>>>>
>>>>> sscanf but also consider the value obtained for %n directive used in sscanf.
>>>>> This ensures that the scanning of malformed inet address isn't flagged as
>>>>> success.
>>>>
>>>> Can you give an example string that would be falsely accepted? To me the
>>>> old checks look fine (even though the new ones are a little bit easier
>>>> to read, so even if they don't fix anything, they might be worth doing).
>>>
>>> "localhost" would fail to be parsed:
>>>
>>> -        if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
>>> +        ret = sscanf(str, "%64[^:]%n:%32[^,]%n", addr, &addr_pos,
>>> +            port, &port_pos);
>>> +        if (addr_pos == -1 || ret == EOF) {
>>>
>>> because the : in the format string would not match and sscanf would
>>> return 1.
>>
>> Yes, that's the part with making the port optional.
>>
>> Bharata also claims that "scanning of malformed inet address" could
>> falsely succeed before, which I can't see (but which I suspect is what
>> the first two hunks of the patch are meant to address).
> 
> For malformed ipv6 address like "[1:2:3:4:5", sccanf in inet_parse
> returns 1 (which means 1 input item successfully matched and assigned)
> The current inet_parse code would eventually fail it since it checks for
> return value of 2, but when I am making port optional, I can't depend
> on return value of 1 or 2 since sscanf can return 1 for such incomplete ipv6
> addresses too. Note that in the above case, though sscanf returned 1, the
> pos argument remains unchanged indicating that it couldn't really parse
> any input correctly.
> 
> So in summary, when I said scanning of malformed inet address succeeded
> earlier, I should have been more specific by saying that sscanf in inet_parse
> could return success for malformed ipv6 strings.

Ah, so you're talking about a potential problem after making the port
optional, not about a real, existing bug in the code as it is today?

Please rephrase the commit message then; actually I wouldn't even care
to talk about the reason for the scanf changes, they are obviously
required for making the port optional.

>>> However, is it correct to set the port unconditionally to an empty
>>> string?  Your usecase makes sense, but perhaps the default port be
>>> passed as an extra parameter to inet_parse instead.
>>
>> I thought about this, too, but didn't care enough to mention it. Now
>> that we're two, yes, I'd like adding a default port parameter.
> 
> So you are saying that lets change inet_parse to look like this:
> 
> int inet_parse(QemuOpts *opts, const char *str, int port)
> 
> and if @str didn't specify a port explicitly, use @port to populate
> the port option in @opts ? Other callers of inet_parse are inet_listen and
> inet_connect. What should be the default port values from these callers ?

Yes, but make it 'int default_port'.

You could make default_port = -1 retain the current behaviour, i.e. port
is required in str (document this in a comment before inet_listen). This
would probably be the right thing to pass for existing callers.

Kevin