#35 Management message response length is off by 2 bytes

v2.3.0
closed
3
2014-07-15
2012-06-28
No

The tlv length field of management message responses is 2 bytes shorter than the actual length that is being sent in the payload, the length should be 2 + N instead of N as stated in the 15.5.2.2. I have a fix for this and will submit the change shortly.

Discussion

  • Todd Newton

    Todd Newton - 2013-04-30

    When management messages are received by PTPd, the application crashes. I suspect this is due to the issue being tracked by this ticket. Has the fix been implemented? I see a fix may have been submitted a couple of months ago, but the ticket status remains "open", leading me to believe that the fix is not yet available in SVN.

    This problem prevents me from using any software that issues management messages.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-04-30

    Todd, I will look into this today. It's possible that my fix did not make it in to the trunk. Will get back to you later this evening.

     
  • George Neville-Neil

    • status: open --> accepted
    • Group: --> v1.0_(example)
     
  • Inaqui Delgado

    Inaqui Delgado - 2013-04-30

    Is this a released version or the trunk? What command line options are you using? Thanks.

     
  • Todd Newton

    Todd Newton - 2013-05-01

    I was running this from the trunk. I believe it was revision #308, which is now a few revisions back. However, it doesn't appear that your update has made it into any of the more recent commits to the trunk.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-05-02

    I'm having issues with my local environment. The daemon can't receive packets, but can send packets, for some reason (getting back EINTR from select in net.c every time). Not sure if its the distro I am using (Scientific Linux 6 64-bit) or a configuration setting that I am not aware of.

    Will hopefully resolve this issue soon and be able to look at the management messages issue.

    In the meantime you can disable management messages by commenting out the call to handleManagement() in protocol.c to prevent your crash. If you still see a crash then it must be something else.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-08-10

    Todd, rev 349 should have a fix for this issue. I wrote a system test to verify the behavior. Trunk rev 348-350 address issues related to management messages. Hope this resolves your issue.

     
  • Todd Newton

    Todd Newton - 2013-10-10

    I've checked out the latest code from SVN (rev 398). PTPd is still having problems when it comes to processing PTP Management Messages. I can build the r398 code and run it just fine. I have an application on another machine that generates PTP Management Messages. However, once the Management Messages are receive, PTPd crashes. Earlier versions (like rev 308) reported a segfault when it received these messages. Looks like it may still be segfaulting.

    The segfaults from rev 308 were seen on a Linux machine. The segfaults from rev 398 were seen on a mac.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-10

    Todd. Can you provide a wireshark capture of the management messages you are sending that causes the crash? This would be helpful so I can use it to write a test, and verify that a code fix solves this problem.

     
  • Todd Newton

    Todd Newton - 2013-10-10

    Inaqui, I've attached a Wireshark capture with the PTP messages. PTPd crashes immediately after it receives one of these Management Messages. I have 2 other hardware slaves in my network, both of which respond fine.

    I also want to point out that I believe the Wireshark dissector for PTP Management Messages is off and wrongly reports malformed packets for these messages. I believe it is off by 2 bytes and is tied to the initial report of this bug. The tlv_length field should be (2 + N), where 2 is the length of the "managementId" and 'N' is the data payload of the message.

    The Management Message that is shown in the capture is a GET for TIME. The payload of TIME is 10 bytes (6 bytes for seconds + 4 bytes for nanoseconds). Thus, the tlv_length is correctly given as 12.

    In general, it would be good for PTPd to gracefully discard any malformed PTP packets rather than segfault (though these messages are not malformed in my network).

     
  • Wojciech Owczarek

    Todd,

    PTPD generally silently discards malformed packets now and increments error counters. The fact it's segfaulting is clearly a bug. Let me see if I can get some PTP management software up and running on my test servers.

     
  • Wojciech Owczarek

    OK, I managed to reproduce the issue, I'm using the packet extracted from the pcap file in this thread.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-10

    I was able to reproduce the issue. The wireshark capture was very helpful. I identified the problem in management.c:handleMMTime. The GET case is not implemented. Will send another update shortly.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-10

    Below is a patch for this issue. I still haven't done thorough testing on it. I would like to do some more testing before committing.

    You are right, the segfault happens in the pack function but the root cause is actually in handleMMTime where memory allocation for the management fields normally occur but in this case there was a TODO to implement this.

    Long term we should consider refactoring the management message code to use a fixed size buffer. It is shortcoming with the first implementation I committed here. I intended to allow users to send as large a data payload as they'd like in mgmt messages but this seems like a bad trade-off since it requires doing memory allocation on the outgoing buffer.

     
    Last edit: Inaqui Delgado 2013-10-10
  • Wojciech Owczarek

    Inaqui,

    I removed my previous comment because I noticed that the GET really wasn't implemented ;)

    So with your patch the responses are being sent correctly and there's no segfault, however tshark/wireshark in the response shows:

        current time (seconds): 90533915000832
        current time (nanoseconds): 916926467
    

    Nanoseconds are OK I think but not too sure about seconds.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-10

    Yes, seconds don't look right. Looking into this further.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-10

    Okay, here's a new patch with an additional fix.

    I will spend some time tomorrow writing/running tests for each management message type to make sure we don't have any other issues that can result in a crash. I'd like to push in the changes tomorrow afternoon assuming my testing doesn't turn up any new issues that I don't have time to fix.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-14

    I submitted revision 402 with my code changes. Please try the trunk when you have a minute.

     
  • Todd Newton

    Todd Newton - 2013-10-14

    Tested and verified with r402. Receiving a ManagementMessage for TIME no longer causes PTPd to segfault. These ManagementMessages are processed and handled appropriately. The proper response is being generated as well.

    This bug can be closed!

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-14

    Great.

    As I was doing testing I did notice a few time related fields(Peer mean path delay, Offset From Master, and Mean Path Delay) are not being populated in some other mgmt messages. I am planning to open a new ticket and submit code to populate these. The fields currently report zero or false but the mgmt responses are fine.

     
  • Inaqui Delgado

    Inaqui Delgado - 2013-10-15

    Rev. 403 populates the Peer mean path delay, Offset from master, and Mean path delay that I mentioned in the previous post. It was a fairly small change. I just uncommented the TODOs and ran tests to verify the information in the management message matched the ptpd debug info for Mean path delay and Offset from master.

     
  • Wojciech Owczarek

    This is really good news. This will be very useful for my production systems.

     
  • Wojciech Owczarek

    • labels: --> management, tlv
    • assigned_to: Inaqui Delgado
    • Group: v1.0_(example) --> v2.3.0
     
  • Wojciech Owczarek

    • status: accepted --> closed
     

Log in to post a comment.