{"action":"created","issue":{"url":"https://api.github.com/repos/plcpeople/nodeS7/issues/26","repository_url":"https://api.github.com/repos/plcpeople/nodeS7","labels_url":"https://api.github.com/repos/plcpeople/nodeS7/issues/26/labels{/name}","comments_url":"https://api.github.com/repos/plcpeople/nodeS7/issues/26/comments","events_url":"https://api.github.com/repos/plcpeople/nodeS7/issues/26/events","html_url":"https://github.com/plcpeople/nodeS7/issues/26","id":224821790,"number":26,"title":"Issue when receiving multiple packets at once","user":{"login":"gfcittolin","id":2856088,"avatar_url":"https://avatars1.githubusercontent.com/u/2856088?v=3","gravatar_id":"","url":"https://api.github.com/users/gfcittolin","html_url":"https://github.com/gfcittolin","followers_url":"https://api.github.com/users/gfcittolin/followers","following_url":"https://api.github.com/users/gfcittolin/following{/other_user}","gists_url":"https://api.github.com/users/gfcittolin/gists{/gist_id}","starred_url":"https://api.github.com/users/gfcittolin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gfcittolin/subscriptions","organizations_url":"https://api.github.com/users/gfcittolin/orgs","repos_url":"https://api.github.com/users/gfcittolin/repos","events_url":"https://api.github.com/users/gfcittolin/events{/privacy}","received_events_url":"https://api.github.com/users/gfcittolin/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-04-27T15:26:34Z","updated_at":"2017-04-28T03:05:59Z","closed_at":null,"body":"We're having an issue, where nodeS7 is treating a packet that is apparently correct as invalid.\r\n\r\n```\r\n...\r\n[997,136871469 172.25.24.210 S2] SendReadPacket called\r\n[997,137073486 172.25.24.210 S2] Sending Read Packet\r\n[997,137322621 172.25.24.210 S2] Sending Read Packet\r\n[997,138683123] INVALID READ RESPONSE - DISCONNECTING\r\n[997,138820445] TPKT Length From Header is 231 and RCV buffer length is 468 and COTP length is 2 and data[6] is 128\r\n[997,138856085] <Buffer 03 00 00 e7 02 f0 80 32 03 00 00 00 02 00 02 00 d2 00 00 04 0b ff 04 00 70 00 00 00 00 00 09 00 00 00 00 00 00 00 00 ff 04 00 70 00 00 00 00 00 00 00 ... >\r\n[997,139296666] ConnectionReset is happening\r\n```\r\n\r\nI've analyzed it a bit and, for the case we have here, it looks like that `onResponse` is being called with two packages in the buffer (maybe the system was a bit slow at the moment) - this explains the TPKT length being 231 and the buffer length being 468 for the example above.\r\n\r\nThe code even treat this special case [here](https://github.com/plcpeople/nodeS7/blob/master/nodeS7.js#L1092), but it looks like the code isn't reaching this anymore because of the fastACK check being mate by `checkRFCData`:\r\n\r\n```javascript\r\n   //...\r\n   }else if((LastDataUnit >> 7) == 1 && TPKT_Length === data.length){\r\n   //...\r\n   }else{\r\n      ret='error';\r\n   }\r\n```\r\n\r\nthis causes `ret` being assigned `'error'`instead of the buffer. Maybe the if-else comparison should be changed to:\r\n\r\n```javascript\r\n   //...\r\n   }else if((LastDataUnit >> 7) == 1 && TPKT_Length <= data.length){\r\n   //...\r\n   }else{\r\n      ret='error';\r\n   }\r\n```\r\n\r\nthus allowing the check previous mentioned check to be executed and succeed.\r\n\r\nI have no experience with this fastACK functionality, therefore I don't know if this could break something. But if you agree on that I can create a PR for the modification above"},"comment":{"url":"https://api.github.com/repos/plcpeople/nodeS7/issues/comments/297895242","html_url":"https://github.com/plcpeople/nodeS7/issues/26#issuecomment-297895242","issue_url":"https://api.github.com/repos/plcpeople/nodeS7/issues/26","id":297895242,"user":{"login":"plcpeople","id":8019526,"avatar_url":"https://avatars1.githubusercontent.com/u/8019526?v=3","gravatar_id":"","url":"https://api.github.com/users/plcpeople","html_url":"https://github.com/plcpeople","followers_url":"https://api.github.com/users/plcpeople/followers","following_url":"https://api.github.com/users/plcpeople/following{/other_user}","gists_url":"https://api.github.com/users/plcpeople/gists{/gist_id}","starred_url":"https://api.github.com/users/plcpeople/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/plcpeople/subscriptions","organizations_url":"https://api.github.com/users/plcpeople/orgs","repos_url":"https://api.github.com/users/plcpeople/repos","events_url":"https://api.github.com/users/plcpeople/events{/privacy}","received_events_url":"https://api.github.com/users/plcpeople/received_events","type":"User","site_admin":false},"created_at":"2017-04-28T03:05:59Z","updated_at":"2017-04-28T03:05:59Z","body":"I spent a while on a similar problem when developing this and the similar Allen-Bradley module as well - the PLC sends out multiple request packets almost at once, and if the responses come in to node at essentially the same time, even if they are separate packets in Wireshark, sometimes node gets two (or possibly more) packets as a single buffer in the 'data' event.  So in this case TPKT_Length is way less than data.length, so we only process the first bytes (as specified in TPKT_Length) and treat the rest as a separate packet which in my earlier testing is processed correctly.  So if RFC expects 231 bytes then the correct thing to do is process the first 231 bytes, assuming it passes validation.  And you call onResponse again with the timeout, which will hopefully validate the next 468-231=237 bytes as another valid packet.  \r\n\r\nI think the fix is correct, unfortunately I don't have anything that sends fast acknowledgement to test the fast ack code.  But it looks to me like it won't be affected by this fix.\r\n"}}
{"actor":{"display_login":"plcpeople"}}