Fix crash in Buffer with null pointer and non-zero length

[accelerated]

When a Buffer is built with null ptr and size > 0, the operator bool return true but any conversion to string crashes.

[mfontanini]

Why would you construct a buffer on a null pointer but with a size larger than 0? Sounds like that construction is just bogus from the user's side.

[accelerated]

It definitely is a bogus construction but in case this happens it's nice not to crash. If the user prints out the buffer and sees empty string instead of some expected value at least the application can continue, you can log the error, and in this case, receive an empty payload.

Alternatively you can throw in the constructor but definitely not assert as it's only good for debug builds. You need some runtime guarantees in release mode as well.

[mfontanini]

I think I would rather throw on construction. A buffer constructed like that is just bogus and here you're basically hiding away the fact that the buffer is broken, so the user won't even realize (assuming they only convert them to strings).

[accelerated]

Ok i made the change. Initially i put a more restrictive check in the constructor to also validate ptr != nullptr && size == 0, which is technically an invalid buffer setting and I ended up getting a crash in Message constructor when EOF is returned from librdkafka. Turns out there's a bug in their code...the documentation says:

typedef struct rd_kafka_message_s {
	rd_kafka_resp_err_t err;   /**< Non-zero for error signaling. */
	rd_kafka_topic_t *rkt;     /**< Topic */
	int32_t partition;         /**< Partition */
	void   *payload;           /**< Producer: original message payload.
				    * Consumer: Depends on the value of \c err :
				    * - \c err==0: Message payload.
				    * - \c err!=0: Error string */
	size_t  len;               /**< Depends on the value of \c err :
				    * - \c err==0: Message payload length
				    * - \c err!=0: Error string length */
	void   *key;               /**< Depends on the value of \c err :
				    * - \c err==0: Optional message key */
	size_t  key_len;           /**< Depends on the value of \c err :
				    * - \c err==0: Optional message key length*/
	int64_t offset;            /**< Consume:
                                    * - Message offset (or offset for error
				    *   if \c err!=0 if applicable).
                                    * - dr_msg_cb:
                                    *   Message offset assigned by broker.
                                    *   If \c produce.offset.report is set then
                                    *   each message will have this field set,
                                    *   otherwise only the last message in
                                    *   each produced internal batch will
                                    *   have this field set, otherwise 0. */
	void  *_private;           /**< Consume:
				    *  - rdkafka private pointer: DO NOT MODIFY
				    *  - dr_msg_cb:
                                    *    msg_opaque from produce() call */
} rd_kafka_message_t;

Essentially when there's an error, the payload contains the error string and the len is that of the error message...in the EOF case len was 0. See opened issue here

I wonder if in this case we should wait for this fix or try to calculate strlen of the error string and leave the Buffer constructor with a more strict setting. Up to you...

[mfontanini]

Well, that makes sense. You should be able to construct a buffer with length 0 and a valid pointer, you could end up doing this if you have some pointer and you decrease your length enough that it reaches 0. Checking for length > 0 is the correct thing to do.

[accelerated]

Yes agreed! The librdkafka bug will stay however as it's incorrectly set.