Add checked exceptions to Runnel decoding and tweak related APIs by ChrisGreenaway · Pull Request #490 · Terracotta-OSS/terracotta-platform

ChrisGreenaway · 2018-09-06T11:38:47Z

No description provided.

lorban

I'm starting to have mixed feelings about this PR. I'm not against the idea of introducing the mandatory field concept, but it's going to make writing multi-version capable codecs much trickier.

Very few fields if any should ever be mandatory, and I'm worried that we mistakenly start setting most if not all fields mandatory.

I wonder if leaving the existing API untouched for the optional fields, and adding a slightly more involved one for mandatory fields (something like decoder.int32() vs decoder.mandatory().int32()) wouldn't be a much better idea to discourage those mandatory fields.

Or am I misguided? An example of multi-version aware codec would certainly help making our mind here.

lorban · 2018-09-06T15:52:34Z

runnel/src/main/java/org/terracotta/runnel/decoding/PrimitiveDecodingSupport.java

@@ -15,7 +15,10 @@
 */
 package org.terracotta.runnel.decoding;


I have a few problems with the changes in this class:

The optional / mandatory prefixes are too long to my taste. I have a feeling that this is going to hurt codec readability.

I have the same kind of feeling towards the optional versions returning Optional<something>. Over time, most fields will become optional because of backward compatibility and I fear that the handling of Optional will be more a nuisance than anything else.

I'm also a little iffy about the performance of the Optional stuff... could we maybe go with:

int int32(String name) throws RunnelDecodingException, int int32(String name, int other) throws RunnelDecodingException

The existing enterprise code implicitly makes some fields mandatory by doing:

long value = decoder.int64("value")

which gives a NPE if the "value" field isn't present.

I'd much rather have it made explicit which fields are mandatory / optional.

I think this will make multi-version codecs easier.

I think most fields will be mandatory. Most fields are currently handled as mandatory, in that they would throw an NPE if they are not present.

I don't think that adding a decoder.mandatory() method to the API will help.

If you have a mixture of optional and mandatory fields then you would end up with something like:

String a = decoder.mandatory().string("a");
Long b = decoder.int64("b");
int c = decoder.mandatory().int32("c2);

The "mandatory().int32()", for example, if longer a involves an extra object compared to mandatoryInt32().

You could save slightly on the optional fields, but it's less explicit that they are optional (NPEs again?) and mandatory fields are probably more common.

I'd be happy to change the "optional" and "mandatory" prefixes to something else if you have a better suggestion. Although, I think they are fine.

Handling an Optional is easier and less error prone than handling the possibility of null. The performance impact of adding Optional is tiny - especially when compared to the network overhead of the message. It's not like we are boxing a primitive up to an Optional. We are working with Objects (like Integer).

The problem with the API Chris D suggested:

int int32(String name) throws RunnelDecodingException, int int32(String name, int other) throws RunnelDecodingException

is that a) it doesn't let you do anything more complex than supplying a default and b) a developers natural tendency will be to use the first version, leading to exceptions for fields that are optional. It's not putting the optionality of the field in your face enough.

I have a slightly different take on this ... at the moment, I'd prefer to have the "mandatory-ness" of a field declared in the struct definition -- it's just as much of an error to omit the value of a mandatory field during encoding as it is to find the field absent during decoding. This gets rid of the mandatory/optional designations in the encoding and decoding methods. It also makes it more clear (at least to me) that, after initial publication, ALL amendments to the struct must be handled as optional fields. with appropriate actions (supply default or flag value; throw field-specific, actionable exception) taken if the field is necessary but missing. (This might actually be helped by methods that're specifically for "needed but missing" fields.)

Playing off of this, we should have unit tests that validate that mandatory struct members haven't been added to keep folks from adding one after initial release.

I toyed with the idea of the struct definition containing the optionality of the fields, but you still end up with needing to change the decoding API to have optional and mandatory versions of the decoding methods because the return types are different (e.g. primitive vs Optional).

The driver for doing it in the struct would be safer coding, rather than security. Runnel would presumably, therefore, throw a RuntimeException (rather than a checked exception) if code tried to encode data that didn't match with the optionality of the fields in the struct.

If one of our codecs encodes data without a mandatory field, that is a different class of error to an attacker deliberately omitting mandatory data.

I think having the optionality declared in the struct is a good idea, but it feels like an orthogonal change to the one this PR is covering.

lorban · 2018-09-06T15:54:04Z

runnel/src/main/java/org/terracotta/runnel/decoding/StructArrayDecoder.java

 * @author Ludovic Orban
 */
-public class StructArrayDecoder<P> implements Iterator<StructDecoder<StructArrayDecoder<P>>> {
+public class StructArrayDecoder<P> {


Since you can't use java.util.Iterator anymore but this class is a close cousin, maybe we should introduce a runnel iterator interface that this (and other array decoders) would implement?

lorban · 2018-09-06T15:55:24Z

runnel/src/main/java/org/terracotta/runnel/decoding/StructArrayDecoder.java

  private final StructField field;

  private StructDecoder<StructArrayDecoder<P>> current = null;
+  private int count = 0;


Why did you introduce this counter?

It's used in the hasNext() method. I didn't want to use the fact that there was no more data in the buffer because, with the counter, we can detect when there wasn't enough data in the buffer to read all the elements that there are supposed to be.

lorban · 2018-09-06T15:56:29Z

runnel/src/main/java/org/terracotta/runnel/decoding/StructDecoder.java


 import java.nio.ByteBuffer;
+import java.util.Optional;



Same as above regarding optional / mandatory prefixes and the Optional return types.

lorban · 2018-09-06T15:58:17Z

runnel/src/main/java/org/terracotta/runnel/decoding/StructDecoder.java

+    return fieldFetcher.fetch(name).orElseThrow(() -> new MissingMandatoryFieldException(name));
+  }
+
+  private interface FieldFetcher<T> {


Add @FunctionalInterface?

lorban · 2018-09-06T16:00:10Z

runnel/src/main/java/org/terracotta/runnel/utils/RunnelDecodingException.java

+ * Indicates there was a decoding problem caused by the data being decoded not matching Runnel's expectations
+ */
+public class RunnelDecodingException extends Exception {
+  protected RunnelDecodingException() {


That constructor shouldn't be needed.

It's used in LimitReachedException. LimitReachedException does not have much to add in the way of message other than the data ran out unexpectedly - which is what the exception type represents.

chrisdennis · 2018-09-07T15:37:10Z

runnel/src/main/java/org/terracotta/runnel/decoding/ArrayDecoder.java

    this.length = readBuffer.getVlqInt();
  }

-  public int length() {


Isn't this solving the problem at too general/low a layer? If we want to be forceful herewe could change the signature to:

public int length(int maximum) throws RunnelDecodingException;

When a developer uses that API, how will they decide what the maximum should be? What if there is more data being sent than the maximum? We would have to go through all the code writing data to do batching on all messages with an array in - a much bigger change.

Otherwise, pretty much the only value a developer will supply as as an argument will be Integer.MAX_VALUE - this does not help. I don't want developers to have to think about these kind of security arguments whenever they are writing a codec.

chrisdennis · 2018-09-07T15:38:09Z

runnel/src/main/java/org/terracotta/runnel/decoding/ArrayDecoder.java

 * @author Ludovic Orban
 */
-public class ArrayDecoder<T, P> {
+public class ArrayDecoder<T, P> implements DecodingIterator<T> {


Iterator isn't anywhere near as useful as Iterable.

Iterable isn't as easy to implement as Iterator :-)

Besides we aren't implementing Iterator, just an interface that looks a bit like it.

I would see moving to an Iterable-like API as outside the scope of this PR. Adding Iterable is adding functionality to the API and is unrelated to security.

I'd be quite happy to get rid of the DecodingIterator interface altogether. None of the enterprise code makes use of the iterability of these decoders.

chrisdennis · 2018-09-07T15:39:44Z

runnel/src/main/java/org/terracotta/runnel/decoding/DecodingIterator.java

+
+import org.terracotta.runnel.utils.RunnelDecodingException;
+
+public interface DecodingIterator<T> {


If we adopt iterable instead of iterator then normal usage patterns would probably make the decoding exception unecessary.

I can't see how that would get rid of the exception. Could you explain?

As far as I can tell, when you go to do the equivalent of next(), you can always encounter a problem with the data (e.g. ran out of bytes). I can't see how this changes with a different API.

chrisdennis · 2018-09-07T15:50:59Z

runnel/src/main/java/org/terracotta/runnel/decoding/PrimitiveDecodingSupport.java

@@ -15,7 +15,10 @@
 */
 package org.terracotta.runnel.decoding;


I'm also a little iffy about the performance of the Optional stuff... could we maybe go with:

int int32(String name) throws RunnelDecodingException, int int32(String name, int other) throws RunnelDecodingException

cljohnso

Is a meeting called for to resolve the mandatory/optional concept handling?

cljohnso · 2018-09-10T15:22:32Z

runnel/src/main/java/org/terracotta/runnel/decoding/fields/Field.java

  int index();

-  void dump(ReadBuffer readBuffer, PrintStream out, int depth);
+  void dump(ReadBuffer readBuffer, PrintStream out, int depth) throws RunnelDecodingException;


It's probably a better idea for this method to handle the RunnelDecodingException internally and write a message to the PrintStream that a decoding error occurred. If I understand the purpose of this method, it's for diagnostics and, in general, diagnostic methods shouldn't throw.

I've now changed dump to write to the PrintStream, as you suggested. I also changed the dump() methods to return boolean rather than void. That way when one dump() method calls another, it knows to abort early if there was a decoding error.

cljohnso · 2018-09-10T15:28:21Z

lease/entity-server/src/main/java/org/terracotta/lease/ActiveLeaseAcquirer.java

+      try {
+        reconnectData = LeaseReconnectData.decode(bytes);
+      } catch (RunnelDecodingException e) {
+        throw new ReconnectRejectedException(e.getMessage(), e);


Is this being logged anywhere? If we have a client feeding the server bad data, we need to make tracks in the log.

This is a good question. The answer is not so obvious. I have sent an email about how these exceptions are handled to @myronkscott and @cschanck.

cljohnso · 2018-09-10T15:33:52Z

runnel/src/main/java/org/terracotta/runnel/decoding/ArrayDecoder.java

+  @Override
+  public T next() throws RunnelDecodingException {
+    if (count >= length) {
+      throw new NoSuchElementException();


I'd be inclined to throw something other than NoSuchElementException here. I know that's what Iterator.next throws but it's a RuntimeException that should be handled in the same way as RunnelDecodingException in code that uses this method (directly or indirectly).

I don't think that a call to next() where count >= length is in the same category.

It's not caused by badly-formatted data - unlike the RunnelDecodingException. It's caused by calling code incorrectly calling next() when hasNext() returned / would have returned false. It's more like an AssertionError.

cljohnso · 2018-09-10T15:36:41Z

runnel/src/main/java/org/terracotta/runnel/decoding/StructArrayDecoder.java


-    if (arrayReadBuffer.limitReached()) {
+    if (count >= arrayLength) {
      throw new NoSuchElementException();


See comment on ArrayDecoder.next().

ChrisGreenaway requested a review from lorban September 6, 2018 11:38

ChrisGreenaway force-pushed the runnel-checked-exceptions branch from cead920 to fc9d96d Compare September 6, 2018 11:59

lorban suggested changes Sep 6, 2018

View reviewed changes

ChrisGreenaway force-pushed the runnel-checked-exceptions branch from fc9d96d to 04824ce Compare September 7, 2018 10:51

ChrisGreenaway requested review from chrisdennis and cljohnso September 7, 2018 12:24

chrisdennis requested changes Sep 7, 2018

View reviewed changes

cljohnso requested changes Sep 10, 2018

View reviewed changes

Add checked exceptions to Runnel decoding and tweak related APIs

62cd5d3

ChrisGreenaway force-pushed the runnel-checked-exceptions branch from 04824ce to 62cd5d3 Compare September 11, 2018 13:12


		import org.terracotta.runnel.utils.RunnelDecodingException;

		public interface DecodingIterator<T> {

Conversation

ChrisGreenaway commented Sep 6, 2018

Uh oh!

lorban left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cljohnso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lorban left a comment •

edited

Loading