Char 's' in received OSC message crashes sclang

[jamshark70]

Environment

• SuperCollider version: 3.10.x
• Operating system: Any

Steps to reproduce

Hat tip Fredrik Olofsson.

// Send to nobody, no problem
NetAddr("127.0.0.1", 57129).sendMsg(\asdf, $s)

// Send to sclang, crash
NetAddr.localAddr.sendMsg(\asdf, $s)

gdb says (it's a rather long stack full of a lot of boost::asio functions -- I'll leave those out because I'm pretty sure the only relevant part is after it comes into sclang at SC_UdpInPort::handleReceivedUDP):

sc3> NetAddr.localAddr.sendMsg(\asdf, $s)
-> a NetAddr(127.0.0.1, 57121)
sc3> 
Thread 2 "sclang" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe3b4f700 (LWP 19686)]
SymbolTable::Make (this=0x555555cdbe40, inName=inName@entry=0x0)
    at /home/dlm/share/supercollider/lang/LangSource/PyrSymbolTable.cpp:136
136     int hash = StrHash(inName, &length);
(gdb) where
#0  0x00005555556bad90 in SymbolTable::Make(char const*) (this=0x555555cdbe40, inName=inName@entry=0x0)
    at /home/dlm/share/supercollider/lang/LangSource/PyrSymbolTable.cpp:136
#1  0x00005555556bae5a in getsym(char const*) (name=0x0)
    at /home/dlm/share/supercollider/lang/LangSource/PyrSymbolTable.cpp:31
#2  0x000055555563ea08 in ConvertOSCMessage(int, char*) (
    inSize=<optimized out>, inData=<optimized out>)
    at /home/dlm/share/supercollider/lang/LangPrimSource/OSCData.cpp:613
#3  0x000055555563fc87 in PerformOSCMessage(int, char*, PyrObject*, int, double) (inSize=<optimized out>, inData=<optimized out>, replyObj=0x55555abb9218, inPortNum=inPortNum@entry=57121, time=time@entry=5.5343247959999999)
    at /home/dlm/share/supercollider/lang/LangPrimSource/OSCData.cpp:698
#4  0x000055555563ff28 in ProcessOSCPacket(OSC_Packet*, int, double) (inPacket=inPacket@entry=0x7fff24000b20, inPortNum=57121, time=time@entry=5.5343247959999999) at /home/dlm/share/supercollider/lang/LangPrimSource/OSCData.cpp:736
#5  0x0000555555711f73 in SC_UdpInPort::handleReceivedUDP(boost::system::error_code const&, unsigned long) (this=0x555555cbecd0, error=..., bytesTransferred=12) at /home/dlm/share/supercollider/lang/LangPrimSource/SC_ComPort.cpp:131
#6  0x0000555555715eda in boost::_mfi::mf2<void, SC_UdpInPort, boost::system::error_code const&, unsigned long>::operator()(SC_UdpInPort*, boost::system::error_code const&, unsigned long) const (a2=<optimized out>, a1=..., p=<optimized out>, this=<optimized out>)
    at /home/dlm/share/supercollider/external_libraries/boost/boost/bind/mem_fn_template.hpp:280
#7  0x0000555555715eda in boost::_bi::list3<boost::_bi::value<SC_UdpInPort*>, boost::arg<1> (*)(), boost::arg<2> (*)()>::operator()<boost::_mfi::mf2<void, SC_UdpInPort, boost::system::error_code const&, unsigned long>, boost::_bi::rrlist2<boost::system::error_code const&, unsigned long const&> >(boost::_bi::type<void>, boost::_mfi::mf2<void, SC_UdpInPort, boost::system::error_code const&, unsigned long>&, boost::_bi::rrlist2<boost::system::error_code const&, unsigned long const&>&, int) (a=<synthetic pointer>..., f=..., this=0x7fffe3b4ebe0)

... snip a ton of boost::asio stuff...

Expected vs. actual behavior

It's perhaps too obvious to mention, but the expected behavior would be that sclang receives the character s without error.

[bgola]

hi! i did some debugging and found a few problems related to this.

First, the sendMsg is not setting the type of the argument to char, it sets the type to the char itself, so when you send an OSC message with $s as argument, it actually sets the type as that char so the OSC message goes with type ,s but there is no string there in the content of the message.

The next problem occurs when parsing messages on the receive side. the OSC parser doesn't check if there is a string there before trying to read it, which means that in the case above it tries to parse a string where there is no data, causing the segfault. this would happen with any message received that is not formatted correctly and is a potential security flaw.

A message with tags ,is but with size 8 (only containing the integer) would also cause a segfault. I think the language should check before trying to parse the OSC message arguments like that. And also set the type as c for chars. There is even a comment from 2009 here: https://github.com/supercollider/supercollider/blob/develop/lang/LangPrimSource/OSCData.cpp#L644, specifying the default rule: if no tag is matched, consider it a char. But this is also inconsistent with the way we treat chars above on line https://github.com/supercollider/supercollider/blob/develop/lang/LangPrimSource/OSCData.cpp#L621

I plan to open two different PRs to deal with both issues, unless someone points something different with it :)

[jamshark70]

I think both could be fixed in one PR.

First, the sendMsg is not setting the type of the argument to char, it sets the type to the char itself...

Seems this was done for some reason, but the reason is not stated. Looks like a hack for something though. Doesn't it basically guarantee a malformed message? If the char is a valid type tag, then you have a type tag without data; otherwise we've added an invalid type tag. I have no idea what we were thinking back then.

https://github.com/supercollider/supercollider/commit/b4cb42146acf7ee4c28f4845b9d1fb73bad49728

[jamshark70]

I remembered later: we use $[ and $] to delimit array arguments in scsynth messages.

x = Synth.basicNew(\default).newMsg(args: [freq: [100, 200, 300]]);
x.asCompileString;
-> [ 9, 'default', 1001, 0, 1, 'freq', $[, 100, 200, 300, $] ]

But sclang's asRawOSC doesn't handle it correctly (you get #bundle and one int only):

y = x.asRawOSC;
-> Int8Array[ 35, 98, 117, 110, 100, 108, 101, 0, 0, 0, 0, 9, 0, 0, 0, 0 ]

So it's definitely a messy implementation. If we're going to use chars as this sort of delimiter, probably better to put a true char in the message instead of abusing type tags (why? to save a few bytes?). But then scsynth will also need to be changed to handle the updated format -- and this has the potential to break non-sclang clients.

Or, allow the type tag hack, but only for square brackets -- all other characters should be inserted as true chars.

[telephon]

It is correct that for the beginning of an array, $[, is added to the type tags. But I suppose that it needs to be added to the message as well.

http://opensoundcontrol.org/spec-1_0 (additional, nonstandard argument types).

Note that there are two longstanding issues in the implementation of the OSC reciever with regards to these nonstandard types as well:

1. array args are not correctly converted to arrays (this can be done via the OpenObject quark, but inefficiently).
2. args that are strings are converted to symbols, filling up the symbol memory when sending arbitrary text (#1029).

[jamshark70]

As I read the spec, it looks like we shouldn't need to add any data for $[ and $] -- just the tags.

So the problem then is that we are treating all characters as the same special case.

I tried a few tests against Pure Data. I couldn't test whether our messages with $[ and $] are well formed because PD doesn't support those type tags. But I could confirm that we are sending malformed messages if the character happens to be a valid type tag that expects an argument to be added as well.

PD: [netreceive -u -b 9696] --> [oscparse] --> [print]

n = NetAddr("127.0.0.1", 9696);

// bracket tags: PD says "hello 1 2"
n.sendMsg("/hello", $[, 1, 2, $]);

// unknown tag silently dropped in PD: "hello 3"
n.sendMsg("/hello", $8, 3);

// valid tag without data: "oscparse: OSC message ended prematurely"
n.sendMsg("/hello", $s, 3);

At the very least, sending malformed messages should be preventable.