RPC messages should sanitize user inputs about sdl_java_suite

[mrapitis]

We have actually discussed this in the past, however it was decided that all parameter validation should occur on the module side (the mobile SDK functions as a pass-through only).

[mikeburke106]

I understand that in order to test the head-unit side (make sure they send INVALID_DATA appropriately, etc), an app should be able to send data that is incorrect. However, I think the open-source mobile libraries should be set up with production mobile applications in mind (rather than head-unit testing). For a production application, each individual developer currently has to sanitize their inputs on their own or else the message will be rejected.

Also, as discussed on Basecamp this week, most production apps on the market don't monitor the RPC responses, which means the application may not even know that the sent message was rejected, leading to confusion and possible incorrect behavior on the head-unit.

Is there any feedback from the open-source community in regards to this topic?

[Heath-FoMoCo]

I don’t think I agree with your stance.

Whether the module or the libraries validate parameters “each individual developer currently has to sanitize their inputs on their own or else the message will be rejected. “ is still a fact. I, and all discussions in the past, have been it is safer to ensure validation on the head unit.

As for “applications not acting on RPC responses” can be handled many other ways. I would prefer a RPC interface which would require a developer to include the response call back.

[joeygrover]

I don't see a reason why both sides don't validate. The mobile side is going to see multiple different module sides with many possible variations, why not send known good data to the module? I think it's safe to assume at one point that some implementation of the module side might not validate correctly or at all, while being an extreme corner case, it's still a possibility. So I'm all for validation on both the mobile and module side since a module will never be able to make good data out of bad data.

[joeljfischer]

I think the point we're trying to make is that it should be as easy as possible for the mobile app developer to send good data, and as difficult as possible to send bad data. The best way to be assured of good data is to check it everywhere that makes sense, which includes on the mobile app side. Letting the app developer know as early as possible of errors and erroneous data is also a good practice implementation of the fail-fast concept.

[mikeburke106]

@Heath-FoMoCo Could you please elaborate on your comment "it is safer to ensure validation on the head unit". I haven't been part of these previous conversations and I'm sure most of the open-source community haven't either, so it would be great to catch us up with the internal conversations.

I believe it would be safer to throw an exception from the library and allow the application to handle it directly as opposed to having the application construct an incorrect message, send the incorrect message to the head-unit and have the head-unit respond with a failed response that is not (currently) monitored by the applications.

I personally agree with Joey's approach - that both the mobile libraries and the core should perform error checking on inputs, making it (nearly) impossible for an application to send incorrect information.

[Heath-FoMoCo]

OK. Let’s get very specific and clear.

There are two types of “validation” that have to happen. One is the message structure. We have to ensure that JSON message is formed and marshallable before sending it to the module. This is something the libraries (specifically the marshalling layer) does, or should do.

The other kind of validation: invalid parameters, invalid characters, char encoding, invalid RPCs, nulls/nills etc. have never been the job of the libraries. This is by design to accommodate expansion, forward compatibility, backward compatibility, implementation differences, etc. This design keeps the libraries thinner, less error/bug prone, and less device intensive.

[mikeburke106]

This enhancement is in regards to the 2nd type of validation you mention (invalid parameters, etc).

I guess I'm confused in regards to your last paragraph. Could you please elaborate further as to how sanitizing user inputs would be more bug prone? Could you provide a specific example? It seems to me that sanitizing user input would make things considerably less bug prone.

As Joel mentioned above, this article lists some good, specific examples for why failing fast is an excellent way to avoid bugs in production code.

[Heath-FoMoCo]

I am equally confused how you can state that adding code and complexity makes a function less bug prone.

I don’t think the debate is about sanitizing user input, but more, where to sanitize it. Having this in multiple places would increase the chance of a bug. The practice has been to have it on the head unit because that is where it HAS to be. Duplicating this would add complexity and therefore bugs.

Regarding the second type of error checking (for SDL semantic correctness,
rather than for data type and marshalability correctness):

I think it is a useful convenience to the developer to have client-side
code do this checking and inform app of errors right away, but that never
guarantees that the request will pass SDL "server" edits, and so even if
client-side code does this checking, the app still has to check async
feedback from SDL server to determine outcome of request, no?

On Fri, Oct 10, 2014 at 2:04 PM, Mike Burke [email protected]
wrote:

This enhancement is in regards to the 2nd type of validation you mention
(invalid parameters, etc).

I guess I'm confused in regards to your last paragraph. Could you please
elaborate further as to how sanitizing user inputs would be more bug prone?
Could you provide a specific example? It seems to me that sanitizing user
input would make things considerably less bug prone.

As Joel mentioned above, this article
http://www.martinfowler.com/ieeeSoftware/failFast.pdf lists some good,
specific examples for why failing fast is an excellent way to avoid bugs in
production code.

—
Reply to this email directly or view it on GitHub
#10 (comment)
.

---

genivi-smartdevicelink mailing list
[email protected]
https://lists.genivi.org/mailman/listinfo/genivi-smartdevicelink

Dave Boll
mobile: [phone]

[joeljfischer]

@Heath-FoMoCo I think the reason we say added code and complexity doesn't make it more bug prone is because the code we are writing is to prevent errors before they occur and to keep ourselves spec-correct. From what I'm understanding of your logic, unit tests (which are additional code) would make the libraries more bug-prone, which is measurably incorrect.

I believe the point of the mobile libraries is to make it as easy as possible for developers is to write correct, good code, and a huge part of that is "fast as possible" error notifications.

The way I imagine the process working is mobile libraries should sanitize the input and report any errors against the spec to the mobile developers. Once the frame is sent to the Head Unit it is run through the PSM to check for additional errors such as transmission and dropped data errors, as well as checking RPC correctness again.

The reason that would be useful is, for instance, if the head unit and mobile libraries are not at the same version level, this would prevent bugs in the mobile library version handling code from propagating through the head unit and causing unexpected bugs in the production environment. Redundancy should only be where appropriate and not just for the sake of it, but in the case of a two-sided transmission, I think it's easy to argue that handing conformance checks on both sides is the safer thing to do than blindly transmitting bad data, or not telling a developer in clear and simple terms where their bugs are.

Even just using basic assertions and exceptions in debug builds (though I would argue using it in production is important too) would be a huge help to mobile developers. They can see within their own IDE right where their bug is instead of looking for bad results on the Head Unit. Let's even say that we make a bug in our library and an assertion is incorrect. The great thing about being open source is that anyone can easily notify us to the bug, or even fix it themselves and push the fix to us (though really that should never happen with unit tests 😸 ).

[mikeburke106]

Here is a specific example of my thought process. Let's say a user creates an AddCommand with cmdID= 10,000,000,000 (obviously not within spec).

AddCommand msg = new AddCommand();
msg.setCmdID(10000000000);
// set menu params, etc and send the message through the proxy

The library allows this even though, according to the spec, it is not allowed.

The head-unit receives the message, parses it and determines that the cmdID is out of range, so it creates an AddCommandResponse with success = false, reason = INVALID_DATA.

However, as mentioned above, the application is not monitoring the onAddCommandResponse callback, so they don't realize the command was not added to the menu.

Now, the developer opens the menu and sees that their AddCommand is not in the menu. They are going to be confused as to why (since they aren't monitoring the callback) and they have absolutely no reference point to where the error is in their code. I can foresee people submitting bug reports on github, thinking that the proxy is at fault, when, in fact, they simply have an ID that is too large.

Whereas, if the library did an error check the application would crash with an InvalidParameterException, giving the developer a reason and a specific line number to investigate why their code isn't working.

I'm going to, again, point to this article, which states:

Some people recommend making your software robust by working around problems automatically. This results in software "failing slowly." The program continues working right after an error, but fails in strange ways later on.
A system that fails fast does exactly the opposite: when a problem occurs, it fails immediately and visibly. Failing fast is a nonintuitive technique: "failing immediately and visibly" sounds like it would make your software more fragile, but it actually makes it more robust. Bugs are easier to find and fix, so fewer go into production.

I think this is a good, concise example of the two methods of thought we've been discussing in this thread.

[Heath-FoMoCo]

Simple examples do not illustrate the whole picture.

That being said I understand your points and (@joeljfischer’s), I understand what can be done, and I understand what might be done to make it easier, better, smarter, quicker, cuter, tastier, etc. for <person_affected> but <world[person_affected]> has to be taken into account to find the best or right solution. The original design of the mobile libraries was intentionally based that the libraries acting as a proxy (pass data from mobile to module) was the right solution at the time. I am not saying this because I designed them, but because that is what I was told when I was introduced to this role. I was difficult to convince that this design was “right” at first, and now, I will be difficult to convince back.

I am not sure of the value of debating on here. If you feel it is a worthy change, please make it and submit a pull request. It will get reviewed by the broader team and be assessed based on risk and benefit.

[jklance]

I'm not sure of the value of making a change and submitting a pull request to such an actively hostile environment for different approaches. Leaving aside, for the time being, the horror I feel in seeing such apparent disdain for the developers that will be using this library; I couldn't imagine putting effort into a submission that has already been shot down with absolutely no real consideration (or only so much consideration as can fit into 24 hours of arguing with people on the internet).

This thread, instead, tells me that if I were to contribute, I'd be better off making a fork. I'm not sure that's the message you're trying to send.

As to the meat of the enhancement; I can totally understand not wanting to write the code, that would take some effort to sanitize the inputs. I can't understand the notion of rejecting sanitization of input as a concept entirely. As a user of libraries and APIs, I actively avoid those that force developers using them to dig for errors when they could provide meaningful responses to error conditions. Do you similarly hate try/catch blocks? Or is it just this form of exception handling that you refuse to allow?

[justinjdickow]

I have another perspective from an engineer, Aaron Tami, whose work focuses on distributed systems at a fairly massive scale. It is in favor of Heath's approach.

Client libraries should be as light weight as possible. Like heath said, if something changes with this you don't want to have to change the validation in two places, it leads to unmanageable code. The fact that these are methods that a third party developer are implementing might make sense in this situation. I don't understand why failing in the client is anymore beneficial than just failing on the server side or the head unit as its called here. When the headunit fails it should provide enough information in the error message to make it clear why the failure occurs. I understand the argument but I don't know, I'm of the school of thought of making client libraries as light weight as possible.

We use this c++ client library that does some validations on inputs before sending the RPC and I had that team make a change for me to allow something new. They made the change but then it still didn't work because the client library had to be updated. This is why things like REST apis are so popular. They're light weight and don't do much except forward what you give it through. Sure, it should validate that data is formatted properly, if it needs to be json, make sure its json.

I don't see why the head unit can't return more information than just INVALID_DATA. Theres usually an error code that represents INVALID_DATA and then an error string that describes what exactly is invalid. In the above situation, such a string would be "10000000000 is not in range, blah blah". Also, not sure if this is applicable for what you're doing, but another argument for light weight client libraries is that, say you have developers interacting with your service using c++, java, js, python - you don't want to have to maintain these validations in each language's library. The head unit just needs to return enough information about the error so that these issues you guys are describing being difficult to debug aren't there.

[mikeburke106]

@Heath-FoMoCo could you please provide a counter-example to help me understand your point of view? I can't think of a situation where having error checking in the code would make the code more error-prone. I understand the history of the design and the rationale behind it, but code should be ever-evolving and improving, so the history of the project shouldn't affect decisions moving forward - especially if there is a potentially better solution.

@jklance , thanks for your feedback. It is good to get some developers from the outside community in on conversations like this since you, as a developer, will ultimately be using this library in your production code. Please stay involved in the conversation, as your feedback is extremely valuable to us.

To Tami's point, I agree that client-side libraries should be lightweight, but when the library contains code like this (from RegisterAppInterface):

public Vector<TTSChunk> getTtsName() {
    if (parameters.get(Names.ttsName) instanceof Vector<?>) {
        Vector<?> list = (Vector<?>)parameters.get(Names.ttsName);
        if (list != null && list.size() > 0) {
            Object obj = list.get(0);
            if (obj instanceof TTSChunk) {
                return (Vector<TTSChunk>) list;
            } else if (obj instanceof Hashtable) {
                Vector<TTSChunk> newList = new Vector<TTSChunk>();
                for (Object hashObj : list) {
                    newList.add(new TTSChunk((Hashtable) hashObj));
                }
                return newList;
            }
        }
    }
    return null;
}

I think the argument that a few extra if statements for error checking makes the library "too heavy" is a pretty weak argument, especially given the huge debugging benefits for application developers that error checking provides.

[joeygrover]

I think Tami's point is absolutely correct when it comes to a single end point system. My fear stems from the fact that instead of a many to one relationship, we will have a many to many. As I stated earlier, I don't think we can guarantee sanitation on the module side will always be the same/correct with every module that is ever created. The mobile libraries have the advantage of being easily updatable and their resources are increasing every iteration.

@mikeburke106 To @Heath-FoMoCo 's point, introducing any extra code into a project increases its chance of having bugs. There really is no way around that. (And that's the argument I believe Heath is making when he says that, correct me if I'm wrong). As a protocol, however, it should theoretically reduce bugs seen in production.

Ok, getting back on track let's work on a pros/cons list of each approach. A quick review of the thread I see these: (Please comment with others that I missed! =] )

Sanitation on Module ONLY
Pros:

• Stays lightweight
• Maintenance cost doesn't increase
• Ability to send module bad data for testing

Cons:

• Developers have to keep track of responses
• Might change between vendors (corner case)

Sanitation Both Sides
Pros:

• Better/faster developer debugging and responses
• Stability increases
• Not wasting transport bandwidth

Cons:

• Maintenance cost will increase
• Time/Resource cost to implement
• Possible extra computations (most likely negligible)

[joeljfischer]

@joeygrover Can you explain the module can send bad data pro for Sanitation on module only? The module can still send bad data to the mobile library, it just will be caught, correct? So there could easily be a debug mode on the mobile device that could bring this bad data from the module to the forefront for debugging, from what I can see.

[joeygrover]

@joeljfischer I think you have it backwards. Without sanitation on the mobile side it's possible to send known bad data to the module. The benefit there is testing the module to make sure it validates that it is bad data, returns a notification to the mobile app, and the app knows how to handle it properly. I would argue this could be achieved through a module testing app rather than the production library, but this is a pro for module side only sanitation because it has that ability right now without any modification or extra work.

[Heath-FoMoCo]

Here is my issue with this:
"Sanitizing user inputs" is such a broad statement I don't even know what it means. We have to get very specific to understand if we would be helping or hurting the environment. Are we talking about "ensuring the developer doesn't exceed upper bounds of IDs", like your example of AddCommand? OK that is fair enough. But what if in 5 years that bound is now 3,000,000 or the Integer.MAX_VALUE? Same thing goes for text lengths. What if a manufacturer wants to make a show 700 chars instead of 500? Now we have to add code to the libraries to book keep what head unit we are connecting to and what the rules are for each head unit.

Are we talking about characters that a head unit can't "render"? OK, we can block tabs or new lines, but then, again, we would be only developing to today. What if someone wants the ability to use new lines?

Are we talking about character encoding? Do we want to keep track of all the possibilities of character encoding for a global product in the libraries?

I am not saying that any of this is not possible, but given the current architecture of the libraries it would be difficult, and bug prone. The libraries was not designed to do this kind of book keeping. I think effort would be better spent creating a library where things like IDs and Correlation were keep by useful robust objects instead of numbers. Lets keep the regions' and different manufactures' head units make their own rules.

Story time:
We have a field in the libraries called NGN Short Name that used to be truncated to 5 chars. This was to accommodate the Navigation Head Unit. One of the first issues we had going global was this "truncation" made a very bad German word prominent on the head unit. We had to go back and revert this change in the libraries and then make sure it didn't negatively affect our GEN 1 NGN and 2 Line head units.

I honestly feel there are bigger and better ways we can make these libraries awesome. I honestly would like us to focus effort on those.