MylesBorins / node-osc

Open Sound Control protocol library for Node.js
GNU Lesser General Public License v3.0
435 stars 72 forks source link

Support for 'c' Type Tag #50

Closed cpvalente closed 2 years ago

cpvalente commented 3 years ago

🐛 Bug report

It seems that node-osc crashes on receiving datagrams of char type 'c'.

throw er; // Unhandled 'error' event
Error: can't decode incoming message: I don't understand the argument code c
    at Socket.<anonymous> (D:\light-dev\ontime\server\node_modules\node-osc\dist\lib\Server.js:28:23)
    at Socket.emit (events.js:315:20)
    at UDP.onMessage [as onmessage] (dgram.js:919:8)
Emitted 'error' event on Server instance at:
    at Socket.<anonymous> (D:\light-dev\ontime\server\node_modules\node-osc\dist\lib\Server.js:29:14)
    at Socket.emit (events.js:315:20)
    at UDP.onMessage [as onmessage] (dgram.js:919:8)

🧐 Expected behavior

Could we look at a library side catch to avoid application crash?

🌍 System information

Using node-osc 6.0.1 in Windows 10 and nodejs 14.15.3

MylesBorins commented 3 years ago

hey @cpvalente

What client are you using that is relying on the char type?

While this was support in 5.x and earlier of OSC I changed the underlying implementation of the decoder to add support for osc bundles which doesn't support the char type.

As per the spec it would appear that c is a non-standard Type Tag... although it also seems like perhaps the appropriate behavior here is to "discard" the unknown type-tag rather than throw. I'll think through the most appropriate behavior here.

I've opened an issue with the upstream dependency I use to decode OSC messages to see if there is interest in implementing additional non-standard osc types.

I could roll back to the older decoder, but it would mean losing support for bundles :(

cpvalente commented 3 years ago

I dont believe rolling back is worth it, I am using an OSC client application and iterate through different messages as a testing platform.

Hopefully there will be a way to ignore unknown types upstream, i will wait for that.

MylesBorins commented 3 years ago

@cpvalente I just pushed a fix in 6.0.2 that will stop node-osc from blowing up if you setup an error handler on the event such as

oscServer.on('error', console.error)

I'll leave this issue open to track both partial decoding of messages with unknown type tags and support for the c type tag

cpvalente commented 3 years ago

just checked with the new version and it looks like the crash persists

MylesBorins commented 3 years ago

Can you try again with the following code

'use strict';
var { Server } = require('node-osc');

var oscServer = new Server(3333, '0.0.0.0');

oscServer.on('message', function (msg) {
  console.log(`Message: ${msg}`);
});

oscServer.on('error', console.error);

I used the oscsend tool that gets installed with liblo to test this

$ oscsend localhost 3333  /sample/address iTfsc 1 3.14 hello a

On receiving the message an error will log, but the process will not crash. If you do not have the error handler setup on the event emitter though it will bubble to the top of the process and cause the process to crash

cpvalente commented 3 years ago

Hi, that doesnt seem to be working on my side. I am using OSC Tester to send the messages

I have added the line as you suggest so my code looks like

import { Server } from 'node-osc';

export const initiateOSC = (config) => {
  const oscServer = new Server(config.port, '0.0.0.0', () => {
    console.log(`OSC Server is listening on port ${config.port}`);
  });

  // error handling
  oscServer.on('error', console.error);

  // message handling
  oscServer.on('message', function (msg) {

I get the following error before the terminal exits

Error: can't decode incoming message: I don't understand the argument code c
    at Socket.<anonymous> (file:///D:/light-dev/ontime/server/node_modules/node-osc/lib/Server.mjs:27:23)
    at Socket.emit (events.js:315:20)
    at UDP.onMessage [as onmessage] (dgram.js:919:8) { address: '10.0.0.12', family: 'IPv4', port: 8888, size: 24 }
file:///D:/light-dev/ontime/server/node_modules/node-osc/lib/Server.mjs:30
      if (decoded.elements) {
                  ^

TypeError: Cannot read property 'elements' of undefined
    at Socket.<anonymous> (file:///D:/light-dev/ontime/server/node_modules/node-osc/lib/Server.mjs:30:19)
    at Socket.emit (events.js:315:20)
    at UDP.onMessage [as onmessage] (dgram.js:919:8)

Inspection with wireshark shows the packetlooking like

0000   02 00 00 00 45 00 00 34 ea 56 00 00 80 11 00 00   ....E..4.V......
0010   0a 00 00 0c ff ff ff ff 22 b8 22 b8 00 20 d2 09   ........".".. ..
0020   2f 6f 6e 74 69 6d 65 2f 67 6f 74 6f 69 64 00 00   /ontime/gotoid..
0030   2c 63 00 00 00 00 00 02                           ,c......

While a string packet looks like

0000   02 00 00 00 45 00 00 34 68 91 00 00 80 11 00 00   ....E..4h.......
0010   7f 00 00 01 7f 00 00 01 22 b8 22 b8 00 20 ac d4   ........".".. ..
0020   2f 6f 6e 74 69 6d 65 2f 67 6f 74 6f 69 64 00 00   /ontime/gotoid..
0030   2c 73 00 00 31 30 00 00                           ,s..10..

This all looks very sane to me, perhaps I am misunderstanding something?

MylesBorins commented 3 years ago

Can you take a look in node_modules/node-osc/package.json and confirm you are using version 6.0.2?

This crash is exactly what I believe I fixed in that release

On Thu, May 27, 2021, 8:59 AM Carlos Valente @.***> wrote:

Hi, that doesnt seem to be working on my side. I am using OSC Tester https://github.com/ETCLabs/OSCTester to send the messages

I have added the line as you suggest so my code looks like

import { Server } from 'node-osc';

export const initiateOSC = (config) => { const oscServer = new Server(config.port, '0.0.0.0', () => { console.log(OSC Server is listening on port ${config.port}); });

// error handling oscServer.on('error', console.error);

// message handling oscServer.on('message', function (msg) {

I get the following error before the terminal exits

Error: can't decode incoming message: I don't understand the argument code c at Socket. (file:///D:/light-dev/ontime/server/node_modules/node-osc/lib/Server.mjs:27:23) at Socket.emit (events.js:315:20) at UDP.onMessage [as onmessage] (dgram.js:919:8) { address: '10.0.0.12', family: 'IPv4', port: 8888, size: 24 } file:///D:/light-dev/ontime/server/node_modules/node-osc/lib/Server.mjs:30 if (decoded.elements) { ^

TypeError: Cannot read property 'elements' of undefined at Socket. (file:///D:/light-dev/ontime/server/node_modules/node-osc/lib/Server.mjs:30:19) at Socket.emit (events.js:315:20) at UDP.onMessage [as onmessage] (dgram.js:919:8)

Inspection with wireshark shows the packetlooking like

0000 02 00 00 00 45 00 00 34 ea 56 00 00 80 11 00 00 ....E..4.V...... 0010 0a 00 00 0c ff ff ff ff 22 b8 22 b8 00 20 d2 09 ........".".. .. 0020 2f 6f 6e 74 69 6d 65 2f 67 6f 74 6f 69 64 00 00 /ontime/gotoid.. 0030 2c 63 00 00 00 00 00 02 ,c......

While a string packet looks like

0000 02 00 00 00 45 00 00 34 68 91 00 00 80 11 00 00 ....E..4h....... 0010 7f 00 00 01 7f 00 00 01 22 b8 22 b8 00 20 ac d4 ........".".. .. 0020 2f 6f 6e 74 69 6d 65 2f 67 6f 74 6f 69 64 00 00 /ontime/gotoid.. 0030 2c 73 00 00 31 30 00 00 ,s..10..

This all looks very sane to me, perhaps I am misunderstanding something?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MylesBorins/node-osc/issues/50#issuecomment-849613175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADZYV7E37YKPJTQG3UDZCLTPY6ZJANCNFSM45RVS7ZA .

cpvalente commented 3 years ago

That is absolutely right, looks like I forgot to clear my node_modules while moving between branches. Error handling works perfectly

MylesBorins commented 3 years ago

@cpvalente glad to hear it. I'll think through ways that we can even further improve the experience... but it might be a little while before I have time to deeply dig in since this might require a rather intensive refactor or internal implementation.

need to review the spec again and maybe consult with some folks I know who helped author it to see what the appropriate behavior should be (e.g. should the rest of the message still come through)

MylesBorins commented 3 years ago

Doing a quick review of the spec again I see it says specifically

an OSC application should discard any message whose OSC Type Tag String contains any unrecognized OSC Type Tags

If we use the IETF definition of "should" I think it is safe to say that the appropriate behavior is to discard the entire message. As this is the current behavior of node-osc, and we no longer crash when there are unknown type tags used I think it is safe to say no action items required on this.

With this in mind I'm going to rescope this issue to be about support specifically for the 'c' (char) typetag

cpvalente commented 2 years ago

I have been actively using your library without any further problems on this. Should we close the issue?

MylesBorins commented 2 years ago

SGTM!