confluentinc / confluent-kafka-dotnet

Confluent's Apache Kafka .NET client
https://github.com/confluentinc/confluent-kafka-dotnet/wiki
Apache License 2.0
2.78k stars 847 forks source link

NullReferenceException setting string properties in the SchemaRegistryConfig to null. #2188

Open bstichter opened 4 months ago

bstichter commented 4 months ago

Description

There is a bug in the way the class SchemaRegistryConfig handles null values for String properties. This exists on multiple properties but was found while setting the SslKeystoreLocation property. The default value of this property is null, but if you try to set the property to null, it throws a NullReferenceException. After decompiling the code, it appears that the underlying logic was written with the intention of supporting null values, but the values are mishandled.

Here is the code for this property:

  public string SslKeystoreLocation
  {
      get
      {
          return Get("schema.registry.ssl.keystore.location");
      }
      set
      {
          SetObject("schema.registry.ssl.keystore.location", value.ToString());
      }
  }

And here is the code for the SetObject() method:

  protected void SetObject(string name, object val)
  {
      if (val == null)
      {
          properties.Remove(name);
      }
      else
      {
          properties[name] = val.ToString();
      }
  }

It is obvious from the logic in SetObject() that support for null values was planned for. Unfortunately, when calling SetObject() the property setter does a value.ToString() which throws the NullReferenceException.

This can be fixed by either 1) adding a null conditional operator to the .ToString() call (i.e. value?.ToString(), or 2) removing the .ToString() entirely. I'm not sure what the purpose is since the value is already a String.

How to reproduce

Executing the following code... anywhere... in any way... throws the exception:

new SchemaRegistryConfig() {
    SslKeystoreLocation = null
}

Here is a Dotnet Fiddle: https://dotnetfiddle.net/4US6Zf

This issue seems to exist in all versions of this file. Based on the change history.

This issue seems to exist for some properties of type String, but not all. As of the latest version of this file in the repo at the time of this writing, this includes SslCaLocation, SslKeystoreLocation, and SslKeystorePassword, but not Url and BasicAuthUserInfo.

Checklist

Please provide the following information:

bstichter commented 4 months ago

I did not check other Config classes to see if this pattern is repeated, but it is possible it extends beyond this class.

rayokota commented 1 month ago

Fixed by https://github.com/confluentinc/confluent-kafka-dotnet/pull/2226