Fix UB in ColumnStringBlock::AppendUnsafe when appending empty string_view

[iliaal]

Summary

ColumnStringBlock::AppendUnsafe calls memcpy(pos, str.data(), str.size()) without short-circuiting on str.size() == 0. When the input string_view was built from an empty std::string, str.data() can be nullptr, and glibc declares memcpy's source pointer with __attribute__((nonnull)) regardless of the size argument. UBSan flags every empty-string append:

runtime error: null pointer passed as argument 2, which is declared to never be null
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clickhouse/columns/string.cpp:139:21

The bug is benign in practice because libc no-ops memcpy(_, NULL, 0), but the false positive fires on any workload that round-trips empty values through String or LowCardinality(String) columns. That makes -fsanitize=undefined builds noisy and hides real findings.

Patch

One-line guard around the memcpy. Behavior is unchanged for non-empty inputs, and the return value matches the previous shape: a string_view pointing at pos with length 0 for empty inputs.

Repro

I hit this in a downstream PHP extension that wraps the library (iliaal/php_clickhouse). Inserting an empty value into a LowCardinality(String) column under -fsanitize=address,undefined reproduces it on every read path. Happy to add a unit test in ut/ if you'd prefer it gated by make test rather than relying on existing coverage.

[CLAassistant]

All committers have signed the CLA.

[Copilot]

Pull request overview

Fixes UBSan-reported undefined behavior in ColumnString::Block::AppendUnsafe when appending an empty std::string_view whose .data() may be nullptr, avoiding memcpy being called with a null source pointer even when the size is 0.

Changes:

• Add a guard to skip memcpy/size update when str.size() == 0.
• Document why the guard is needed (UBSan + memcpy nonnull attribute behavior).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

const auto pos = &data_[size]; can be undefined behavior when size == capacity (e.g., appending an empty string when the block is full, or when capacity==0), because data_[size] conceptually dereferences one-past-the-end. Prefer computing the pointer via data_.get() + size (and consider applying the same change in the other Block helpers that use &data_[size]), so zero-length operations don’t rely on out-of-bounds operator[] evaluation.

[Copilot]

The comment suggests an empty std::string may have data() == nullptr, but standard std::string::data() is generally non-null even when empty. The nullptr case is definitely possible for an empty std::string_view (e.g., default-constructed std::string_view{} or a view built from a nullable C string), so it would be clearer/safer to describe the issue as “empty string_view can have null .data()” rather than tying it specifically to std::string().

Suggested change

	// size argument, so an empty string_view backed by std::string()
	// (where data() may be null) trips UBSan on every empty append.
	// size argument, so an empty string_view with a null data() pointer
	// trips UBSan on every empty append.