Skip to content

Fix NPE in KeyValue when the message payload is null#10085

Closed
vroyer wants to merge 1 commit intoapache:masterfrom
vroyer:support_keyvalue_with_null_value
Closed

Fix NPE in KeyValue when the message payload is null#10085
vroyer wants to merge 1 commit intoapache:masterfrom
vroyer:support_keyvalue_with_null_value

Conversation

@vroyer
Copy link
Contributor

@vroyer vroyer commented Mar 30, 2021

Motivation

Fix a NullPointerException when decoding a KeyValue with a null message payload.

Modifications

Return a KeyValue(null, null) when data is null or empty.

Verifying this change

Add a unit test in org.apache.pulsar.common.schema.KeyValueTest

private final K key;
private final V value;

public static final KeyValue NULL = new KeyValue(null, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this private ?

* @return the decoded key/value pair
*/
public static <K, V> KeyValue<K, V> decode(byte[] data, KeyValueDecoder<K, V> decoder) {
if (data == null || data.length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please explain how it can happen that we fall into these cases ?

I mean, probably data == null is possible in case of a message will NULL value, but I cannot think about a case in which the array may be of zero length.

probably in case of zero length array we should throw an IllegalArgumentException, because it is not possible that it is a valid value, as we are always writing the length of the key and the length of the value (with -1 as marker for nulls)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appends when key is SEPARATED and value is null, here is the KeyValueSchema.encode():

public byte[] encode(KeyValue<K, V> message) {
        if (keyValueEncodingType != null && keyValueEncodingType == KeyValueEncodingType.INLINE) {
            return KeyValue.encode(
                message.getKey(),
                keySchema,
                message.getValue(),
                valueSchema
            );
        } else {
            if (message.getValue() == null) {
                return null;
            }
            return valueSchema.encode(message.getValue());
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vroyer I still do not see how we can have a zero byte array from your example.

if we have keyValueEncodingType == KeyValueEncodingType.SEPARATED then we are not using the KeyValue encoding, but the value contains only the serialized version of the value (valueSchema.encode(message.getValue()) ).

this "decode" code should be used only when you read an KeyValueEncodingType.INLINE message, that contains both the key and the value inside the payload of the message:

  • keylen
  • key bytes
  • valuelen
  • value bytes

when you serialize a null KeyValue you can a null value
when you serialize a null key you see -1 as "keylen"
when you serialize a null value you see -1 as "valuelen"

there is no way to see a zero byte array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, #10089 is possibly the root cause. The KeyValue with a null value was decoded as INLINED, so this protection https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/KeyValueSchema.java#L146 was not working as expected.

import org.apache.pulsar.client.impl.schema.StringSchema;
import org.apache.pulsar.client.impl.schema.TimeSchema;
import org.apache.pulsar.client.impl.schema.TimestampSchema;
import org.checkerframework.checker.units.qual.K;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this import

import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.util.ArrayList;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this import looks unused

import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;

import com.google.common.collect.ImmutableList;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this import looks unused

@vroyer vroyer force-pushed the support_keyvalue_with_null_value branch from b4ac2c6 to 371ccbb Compare March 30, 2021 11:17
@eolivelli
Copy link
Contributor

it looks like we can close this PR in favour of #10089

correct ?

@sijie sijie added this to the 2.8.0 milestone Mar 30, 2021
@Test
public void testNullPayload() {
KeyValue kv = KeyValue.decode(
null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to test empty data array?

Schema.STRING.decode(valueBytes)
)
);
assertNotNull(kv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to assert the key & value of this kv?

@eolivelli
Copy link
Contributor

this patch is obsolete, we can close this PR

@eolivelli eolivelli closed this May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants