Re: [erlang-questions] Bug in epmd_srv.c?

[Klas Johansson]

Hi,

On Thu, Sep 3, 2009 at 11:24 PM, Dave Smith <[hidden email]> wrote:

> Hi all,
>
> I've been working on a helper module to enable usage of epmd
> for things other than just nodes (i.e. poor man's zeroconf
> service). In the process, I've found some discrepancies in the
> epmd documentation and server code. I _think_ there is a bug or
> two there, but would appreciate validation by the OTP team --
> it could just be a lack of understanding on my part. :)
>
> First off, the docs for the distribution protocol state that
> the ALIVE2_REQ has the field "LowestVersion" followed
> by "HighestVersion".  However, the implementation in
> erl_epmd.erl has them reversed as does the documentation for
> the PORT2_RESP packet. This is, I believe, a minor
> documentation bug.

Now I too have been bitten by this.  This patch places the
`highest version' field before the `lowest version' field in the
docs, just as in the source code:

    git fetch git://github.com/klajo/otp.git epmd_alive2req_version_ordering

> Secondly, in erts/epmd/src/epmd_srv.c:628 (R13B01), there is
> the following line:
>
>     offset += (strlen(node->extra)-1);
>
> Unfortunately, if the length of the extra field is zero, this
> causes the last byte of the packet to disappear as it winds up
> decrementing "offset".
>
> Reading through epmd, it looks like the expectation is that the
> extra field is null-terminated --- but the docs make no mention
> of it. This is somewhat odd to me as that field is prefixed
> with a 2 byte length value in the protocol, so I see no reason
> for the implementation to require the data to be a C string. Of
> course, this may be for backwards compatibility purposes; it's
> hard to know for sure.

I stumbled upon this one as well and had a go at fixing it.
Before the change I discovered that (according to docs, fields
are a two-byte length followed by the value itself [1]):

* <<0>> instead of <<0, 0>> was returned when extra was <<>>
  (truncated)

* <<0, 4, 4, 1, 2>> instead of <<0, 4, 1, 2, 3, 4>> was returned
  when extra was <<1, 2, 3, 4>> (truncated and garbled; actually
  the second byte of the length is duplicated)

The fix tries to address that.  I'm not too sure about the fix
though, since this is in some sense not a backwards compatible
fix (since extra and it's length will be different).  However,
since the previous version truncated (and garbled) the "extra"
field and erl_epmd.erl doesn't seem to care about it it might not
be that big a deal.  Perhaps for some other third-party libraries
or other parts of Erlang/OTP (ei, jinterface, ...; I haven't
checked)?  Also, I'm not that confident in modifying the
epmd... :-) I've attached a test case which illustrates the
problem.  Get the code here:

    git fetch git://github.com/klajo/otp.git epmd_port2resp_trunc_extra

I'd be happy to hear from someone who's more well-versed in the
epmd and the distribution protocol.


Kind Regards,
Klas

[1] http://www.erlang.org/doc/apps/erts/erl_dist_protocol.html


________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org

[Dave Smith-10]

On Sun, Dec 20, 2009 at 3:06 PM, Klas Johansson
<[hidden email]> wrote:

> The fix tries to address that.  I'm not too sure about the fix
> though, since this is in some sense not a backwards compatible
> fix (since extra and it's length will be different).  However,
> since the previous version truncated (and garbled) the "extra"
> field and erl_epmd.erl doesn't seem to care about it it might not
> be that big a deal.  Perhaps for some other third-party libraries
> or other parts of Erlang/OTP (ei, jinterface, ...; I haven't
> checked)?  Also, I'm not that confident in modifying the
> epmd... :-) I've attached a test case which illustrates the
> problem.  Get the code here:

It's been a while since I've looked at this, but at first glance it
seems to me that while your patch solves the case where extra is a
typical null-terminated string, it does not address the problem where
extra has binary data with nulls sprinkled throughout. To deal with
this, you'd need to add a field to track the length of extra (instead
of depending on strlen) and also replace the strcpy on 627 to a
memcpy.

Hope that helps. :) I am very glad to see someone else looking at it.
Extra would be quite useful to have working properly.

D.

________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org

[Klas Johansson]

Hi Dave,

On Mon, Dec 21, 2009 at 4:01 AM, Dave Smith <[hidden email]> wrote:

> On Sun, Dec 20, 2009 at 3:06 PM, Klas Johansson
> <[hidden email]> wrote:
>
>> The fix tries to address that.  I'm not too sure about the fix
>> though, since this is in some sense not a backwards compatible
>> fix (since extra and it's length will be different).  However,
>> since the previous version truncated (and garbled) the "extra"
>> field and erl_epmd.erl doesn't seem to care about it it might not
>> be that big a deal.  Perhaps for some other third-party libraries
>> or other parts of Erlang/OTP (ei, jinterface, ...; I haven't
>> checked)?  Also, I'm not that confident in modifying the
>> epmd... :-) I've attached a test case which illustrates the
>> problem.  Get the code here:
>
> It's been a while since I've looked at this, but at first glance it
> seems to me that while your patch solves the case where extra is a
> typical null-terminated string, it does not address the problem where
> extra has binary data with nulls sprinkled throughout. To deal with
> this, you'd need to add a field to track the length of extra (instead
> of depending on strlen) and also replace the strcpy on 627 to a
> memcpy.

True.  I've made a new version, which takes that into account.  I
introduced a new field into the Node/enode struct which remembers
the length of the "extra" when a node is registered in the epmd,
and uses that length together with a memcpy when building the
PORT2_RESP.

I'd love to hear from the Erlang/OTP team and their take on this.
I can push it back to github later on today, if there are no
other things to take into account.

> Hope that helps. :) I am very glad to see someone else looking at it.
> Extra would be quite useful to have working properly.

:-)  Thanks for your feedback.


BR,
Klas

________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org

[Björn Gustavsson]

On Sun, Dec 20, 2009 at 11:06 PM, Klas Johansson
<[hidden email]> wrote:
> Now I too have been bitten by this.  This patch places the
> `highest version' field before the `lowest version' field in the
> docs, just as in the source code:
>

Thanks!

I have applied your patch directly to the development branch
(it will turn up later today in the git repository), with one minor
change (referring to the next field for possible values no longer
make sense, so I copied the line "The value in R6B and later is 5"
from the other version number).

>> Secondly, in erts/epmd/src/epmd_srv.c:628 (R13B01), there is
>> the following line:
>>
>>     offset += (strlen(node->extra)-1);
[...]

I'll wait for your updated version.

--
Björn Gustavsson, Erlang/OTP, Ericsson AB

________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org

[Klas Johansson]

2009/12/21 Björn Gustavsson <[hidden email]>:

> On Sun, Dec 20, 2009 at 11:06 PM, Klas Johansson
> <[hidden email]> wrote:
>> Now I too have been bitten by this.  This patch places the
>> `highest version' field before the `lowest version' field in the
>> docs, just as in the source code:
>>
>
> Thanks!
>
> I have applied your patch directly to the development branch
> (it will turn up later today in the git repository), with one minor
> change (referring to the next field for possible values no longer
> make sense, so I copied the line "The value in R6B and later is 5"
> from the other version number).

Ok, great!

>>> Secondly, in erts/epmd/src/epmd_srv.c:628 (R13B01), there is
>>> the following line:
>>>
>>>     offset += (strlen(node->extra)-1);
> [...]
>
> I'll wait for your updated version.

I've amended my previous patch to incorporate the fix for the
what-if-extra-contains-null-characters issue.  Get it here:

   git fetch git://github.com/klajo/otp.git epmd_port2resp_trunc_extra

I've also attached a new version of the erlang test case which
tests the null issue as well.  I'd be happy to hear what you
think.

BR,
Klas


________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org

[Björn Gustavsson]

2009/12/22 Klas Johansson <[hidden email]>:

> I've amended my previous patch to incorporate the fix for the
> what-if-extra-contains-null-characters issue.  Get it here:
>
>   git fetch git://github.com/klajo/otp.git epmd_port2resp_trunc_extra

Thanks! Included in 'pu'.

Is there any special reason to fix this bug? I mean, besides
fixing a bug, is there anything that will work better or something
that now will be possible to do that was not possible to do
before, or does it increase security?

If the answer to any of those questions is "yes", it would be
good to mention it in the commit message.

> I've also attached a new version of the erlang test case which
> tests the null issue as well.

It would be nice if you could turn that code into a test case.

> I'd be happy to hear what you think.

We will come back to you next year. Unless we uncover any
backward-compatibility issues or other problems, this fix
should make it into R13B04.

--
Björn Gustavsson, Erlang/OTP, Ericsson AB

________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org

[Klas Johansson]

Hi,

Sorry for the delay...

2009/12/22 Björn Gustavsson <[hidden email]>:
> 2009/12/22 Klas Johansson <[hidden email]>:
>
>> I've amended my previous patch to incorporate the fix for the
>> what-if-extra-contains-null-characters issue.  Get it here:
>>
>>   git fetch git://github.com/klajo/otp.git epmd_port2resp_trunc_extra
>
> Thanks! Included in 'pu'.

I've seen that it has graduated now, great!

> Is there any special reason to fix this bug? I mean, besides
> fixing a bug, is there anything that will work better or something
> that now will be possible to do that was not possible to do
> before, or does it increase security?
>
> If the answer to any of those questions is "yes", it would be
> good to mention it in the commit message.

I see what you're asking for here, but I was just fooling around with
the distribution protocol and another programming language.  Perhaps
Dave has some input here?

>> I've also attached a new version of the erlang test case which
>> tests the null issue as well.
>
> It would be nice if you could turn that code into a test case.

I've done a new commit here (where I've added two more testcases to
epmd_SUITE.erl):

    git fetch git://github.com/klajo/otp.git epmd_port2resp_trunc_extra

I didn't amend my previous commit, since it had already graduated from 'pu'.


Thanks and Regards,
Klas

________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org

[Björn Gustavsson]

2010/1/24 Klas Johansson <[hidden email]>:
> I've done a new commit here (where I've added two more testcases to
> epmd_SUITE.erl):
>
>    git fetch git://github.com/klajo/otp.git epmd_port2resp_trunc_extra
>

Thanks! I have added your commit directly to ccase/r13b04.

--
Björn Gustavsson, Erlang/OTP, Ericsson AB

________________________________________________________________
erlang-patches mailing list. See http://www.erlang.org/faq.html
erlang-patches (at) erlang.org