[C++] ArraySpan::FillFromScalar is unsafe

[bkietz]

Describe the bug, including details regarding any error messages, version, and platform.

ArraySpan::FillFromScalar can store pointers into the structure itself (specifically, into ArraySpan::scratch_space) which produces an ArraySpan which easily be unsafely moved and copied:

ArraySpan span;
span.FillFromScalar(scalar);

UseSpan(span); // Fine; the original span is still alive.

return span; // Undefined Behavior; the returned copy views
             // a stack variable whose lifetime has ended.

The capability to view a Scalar as an ArraySpan can be preserved and made safer by restricting access to the span to an explicitly delineated scope:

ArraySpan::FillFromScalar(scalar, [&](ArraySpan span) {
  UseSpan(span); // Fine; the viewed scratch space is alive inside this scope.
});

This has the pleasant side effect of reducing the size of ArraySpan by 16 bytes.

Component(s)

C++

[felipecrv]

ArraySpan span;
span.FillFromScalar(scalar);

UseSpan(span); // Fine; the original span is still alive.

return span; // Undefined Behavior; the returned copy views
             // a stack variable whose lifetime has ended.

Due to RVO, the returned ArraySpan is constructed at the caller's stack and this is not biting us, I guess. I think this is a matter of always using destination-passing style when dealing with ArraySpan and never copying or moving it.

Copy and Move constructors haven't been = delete;d in the class, I wonder if they could be still. I think we are allowed to do so as it's laveled EXPERIMENTAL.

[bkietz]

This is not biting us currently because AFAICT the only place we use FillFromScalar is

arrow/cpp/src/arrow/compute/exec.cc

Lines 319 to 330 in cd6e2a4

	void PromoteExecSpanScalars(ExecSpan* span) {
	// In the "all scalar" case, we "promote" the scalars to ArraySpans of
	// length 1, since the kernel implementations do not handle the all
	// scalar case
	for (int i = 0; i < span->num_values(); ++i) {
	ExecValue* value = &span->values[i];
	if (value->is_scalar()) {
	value->array.FillFromScalar(*value->scalar);
	value->scalar = nullptr;
	}
	}
	}

where they are in a std::vector from which they are never moved. This is subtle too: as long as the original ArraySpan remains valid copies will be valid too, so

ArraySpan Thing::get(int i) { return vector_of_spans_[i]; }

Might not segfault if the Thing happens to be kept alive.

[felipecrv]

Scary. And as I thought more about my proposal of deleting the copy ctor of ArraySpan, I realized that completely defeats the point of having a non-owning span type.

If we really care about small buffers being cheap, we should instead consider changing the Buffer class to have an optimization similar to the small-string optimization that std::string does. That can be made safe by correct implementation of the move/copy constructors of Buffer

[pitrou]

Given that ArraySpan is non-owning, isn't it part of the API contract that you have to make sure the backing object does not fall out of scope?

We should probably make this clearer in the docstrings, though.

[bkietz]

It's more serious than that since the ArraySpan does own the scratch space, which means that copies are viewing data in a scalar and also data in the original span (which may not exist anymore)

[raulcd]

I am moving this to 14.0.0 as it is not marked as a blocker for the 13.0.0 release, let me know if it should be part of the release though.