OSC interface: Please support N (nil) type tag

[jamshark70]

Currently, nil in an OSC message array is converted to an integer 0 in the outgoing message. This makes it impossible for the receiver to know if it's a legitimate attempt to send 0, or a failure condition.

This is definitely occurring in sclang's OSC primitives:

["/b_free", nil].asRawOSC
-->
Int8Array[ 47, 98, 95, 102, 114, 101, 101, 0, 44, 105, 0, 0, 0, 0, 0, 0 ]

... where 105 is the type tag for 'i' and the data bytes are 0. Further, if you send such a message (as below) to, say, PD, you'll cleanly get ["/b_free", 0] on the other side.

n = NetAddr("127.0.0.1", 9997);
n.sendMsg(\b_free, nil);

Now... why am I picking on /b_free? Because here is a case where you absolutely do not want the server to take action on wrong information. But, if you free a Buffer object and, sometime later, free it again, Buffer will build exactly the message ["/b_free", nil] -- which will be sent to the server as ["/b_free", 0] whereupon any data that you have carefully stored in buffer 0 will be destroyed.

I've put in a pull request to stop Buffer from building/sending a "free" message in this case, but this won't help other clients. I think the only comprehensive solution is to send the message to the server exactly as it was built, nil and all, and let the server fail the command and send the failure response back to the client.

In most cases, a 0 default doesn't do much damage. But in this case... I could fill the entire next screenful with expletives and still not manage to express exactly how thoroughly, appallingly unacceptable the current situation is.

We take too many shortcuts in interface design, and the users end up paying for it.

[jamshark70]

It seems to be pretty easy to get sclang to send nils: at https://github.com/supercollider/supercollider/blob/master/lang/LangPrimSource/OSCData.cpp#L189, move case tagNil to a separate branch, and this branch should do packet->addTag('N'); break; -- in OSC, nil is represented by the tag only, no data, so this seems to be sufficient on the sending side.

s.dumpOSC(1);
s.sendMsg(\testing, \a, nil, \b, 1);

With that proposed change, the server prints nil in the right place in the message. So at least some part of the server can interpret the nil tag. However (edit: setMsg instead of set):

s.reboot;

b = Buffer.alloc(s, 1, 1, completionMessage: { |buf| buf.setMsg(0, 1) });

b.get(0, _.postln);  // 1, OK

s.sendMsg(\b_free, nil);  // dumpOSC prints [ "/b_free", ] here

b.get(0, _.postln);  // index out of range !!!

So we have to decide what to do with nil for each message argument. I think in most argument slots, nil should cause a failure to be sent back. In a synth argument list, a nil for the name/index or value should cause that pair to be dropped (falling back to the SynthDef's default for that control). Nil in an arrayed control, not sure...

Also, I haven't looked yet at sclang receipt of nil in a message.

[jamshark70]

Server-side: I found where the inappropriate 0s are introduced, but a proper fix is beyond my c++ ability.

int BufFreeCmd::Init(char *inData, int inSize)
{
    sc_msg_iter msg(inSize, inData);
    mBufIndex = msg.geti();
...
}

The buffer index comes from a geti() function, declared in include/plugin_interface/sc_msg_iter.h:

    int32 geti(int32 defaultValue = 0);

int32 means the function has to return a 32-bit integer... but now we need it to return either a 32-bit integer, or failure.

What's the best c++ mechanism for this? I suppose either one of these, but I don't know which is better. Both are intrusive, requiring changes in all command handlers (including third party extension /b_gen plug-ins... not commonly used, but there is one associated with PartConv as I recall).

• Throw an exception (and wrap every geti() call in a try block);
• Or change all of the sc_msg_iter member functions to return a success or failure status, and pass the value back by reference.

A nasty cheat would be just to change the default value to -1 -- then at least it would not free a buffer that you didn't explicitly ask to free. But... wow, that's ugly, and IMO the point of this bug report is to fix it properly.

c++ gurus...?

[muellmusik]

Well, as you've noticed, currently the way scmsgiter deals with wrong types is to return a default value. Arguably that's not ideal, as it can hide a class of errors. What you should be doing here is checking for correct type in every message, and 'failing' if the type is wrong. In doing this, you'd need to maintain existing tolerances (e.g. cast from float to int and vice versa) to avoid breaking code.

Your cheat is no good. I think you're looking at option two, rather than throwing an exception, which seems like overkill. I'd have to look at the bgen case to see about compatibility, but as there is an API version for plugins this could be coped with. (Though incrementing the API version is a heavy change.)

You will also need to rewrite all the commands, taking care to maintain the defaults specified in the Server Command Reference.

[muellmusik]

What you really need is an error code indicating, wrong type, too few args, ...?

[jamshark70]

Yeah, I'm realizing this is a big nightmare. Perhaps that's why the shortcut in the interface was taken in the first place. I'm not going to have time for that much work all by myself, not in the near future.

On the other hand, it occurs to me that all _set messages are potentially risky -- say you create a whole bunch of NodeProxies and then accidentally "set" a node whose ID has been cleared, and to make it really icky, say it was node.set(\out, 0). Boom! The set message goes to node 0, clearing the signal routing for every synth.

It's perhaps miraculous that nobody has reported it in the 14 or so years that I've been around here.

Dunno, it's a bad issue, but on the other hand, nobody's seen it (and, btw, how is it that I stumble onto one of these issues just about every week while other people are making music?) so maybe not much harm if it waits awhile.

In the meantime, I'm going to add a ServerBoot action into my local environment to reserve buffer ID 0 in advance so that I never use 0 for anything important.

[muellmusik]

Perhaps that's why the shortcut in the interface was taken in the first place.

I think it was actually to allow for defaults. The Cmd handling code can pass defaults into the get funcs.

[timblechmann]

supernova initially had osc type checking, which caused quite some troubles and broke existing code, which expects type mismatches to fall back to the defaults.

[jamshark70]

As I recall, some of supernova's type checking would reject floats where integers would be expected.

That isn't the issue here.

[jamshark70]

I think it was actually to allow for defaults. The Cmd handling code can pass defaults into the get funcs.

In that case, BufFreeCmd could simply pass a default that will not accidentally destroy user data, couldn't it? And this would not affect defaults for other commands where a 0 default would not be damaging?

[muellmusik]

In that case, BufFreeCmd could simply pass a default that will not accidentally destroy user data, couldn't it? And this would not affect defaults for other commands where a 0 default would not be damaging?

No, I think it's more complex than that. Essentially you need to know what happened, i.e. did the user pass no arg? explicit nil? a wrong type (for this message)? Maybe, was there a cast from something to the expected type? You might handle these differently. Just passing a default that 'doesn't destroy' user data might actually be worse than the current situation, as it could make it harder to discover.

I think if this is going to be done it should be done right, not just a fix for some particular class of use cases.

[jamshark70]

I hope I wasn't giving the impression that I'm actively looking for shortcuts. That is certainly not my intent. I had hoped that the at least 3 pejoratives in the "cheat" paragraph would have been sufficient to convey my views, but I might have undone those negatively-loaded words with my last question, so let me be explicit: Yes, fix it properly, or leave it alone and document that you can accidentally really screw things up.

But, if it's up to me to do all the coding, then nothing will happen until the summer. This isn't a quick fix.

[telephon]

Wouldn't it be possible to just properly support the nil OSC type tag? http://opensoundcontrol.org/spec-1_0

[muellmusik]

Wouldn't it be possible to just properly support the nil OSC type tag?

Yes, but the bigger question is that the current scheme leads to unexpected behaviour with incorrect data types. e.g. pass a string for an arg instead of a num and you get the default.

[jamshark70]

Beyond what Scott said: it's impossible to support OSC nil without a failure mechanism. Currently the server spec says, e.g., for b_free, it is going to try to delete a buffer, even if you didn't give a buffer number and even if it makes no sense to continue in that case. The current OSC parser actually makes it impossible to distinguish cases where it should fail.

I'm still not sure who is actually going to do this, though. If it all falls to me, I'm estimating about 15 hours (to update and test each of some 30 server commands -- given the number of commands, 15 hours may be optimistic). Realistically, I don't have that kind of time.

So we probably should figure out some sort of plan B. IMO it's not good to leave it as it is, but if the only plan to move forward is that James blows a week of his summer on somebody else's suboptimal design decision, then it ain't gonna happen.

[jamshark70]

Plan B could be, I figure out the shape of a fix and then we distribute the grunt work to a handful of devs.

[vivid-synth]

Isn't another plan B that we simply throw an exception if the user tries to encode a nil as OSC? That way you won't be freeing the wrong buffer or calling node.set(\out, 0), and none of the messages in "Server Command Reference" use a nil. User-defined stuff with e.g. /u_cmd could break, but it's worth asking on the list if anyone actually has a use for nil (and of course, using ['i', 0] is a backwards-compatible fix!)

Also, while we're poking around this neck of the code, the cases for tagPtr in lang/LangPrimSource/OSCData.cpp feels not-great. Sending ['i', 0] for a pointer (that is what it is, correct?) seems like the same "cheerily doing the wrong thing and maybe breaking things" @jamshark70 brought up to begin with.

(Side note:) We might also want to consider the cases of tagFalse and tagTrue -- OSC has support for 'False' and 'True'; is there a reason we're sending them as ints?

[jamshark70]

Counterexample: s.sendMsg(\b_free, "oops this should be a number but really it's a string"). The server will delete buffer 0 in this case too.

I suppose the client could type-check server messages, but it would have to avoid throwing exceptions for messages that appear to be for scsynth while actually going to other processes. That seems difficult and easy to break.

[vivid-synth]

My operating assumption when writing Vivid was to do all data checks before the message got to scsynth/supernova, because they (at least scsynth) are super fragile. Lots of malformed OSC messages will cause segfaults and other nastiness.

That said, 's.sendMsg' should maybe have fewer checks on it than higher-level functions (e.g. n.set), because like you say they can be sending to other processes. The checks for the correct type of the messages could be in the functions (n.set etc) -- the downside is this is more work than just banning all nils in all osc messages.

[telephon]

@vivid-synth:

Many of the assumptions of the sc-server engine are kind of low level analogs of the dynamic (aka duck) typing of sclang – and this is intentional (even if it is decision in a trade-off). Part of that logic is that the meaning of a message is with the receiver, not the sender. As far as I can see, it therefore should be in message reception of sc-synth where a default value is defined, or a type error is thrown. We shouldn't have to check on the sender side.

As you discovered segfaults and other nastiness, because you are haskell-fuzzing the server in the right way, maybe it would be good if you can report them, maybe they can be easily fixed. A few years ago, I fuzzed the server with random messages and could get rid of a few problems, but as you observed, seem to be still many open.

[telephon]

in other words, b_free shouldn't have a default value of 0!

[muellmusik]

I can do the fix on this. It will take me less than 15 hours I'm sure, but I'd appreciate it if others could handle testing, and really write a series of Unit Tests for server commands which include these inappropriate values

[muellmusik]

This is old, but seems like it should be fixed, so leaving open.