`CometBuffer` can potentially lead to concurrent modification of a held buffer (aka is "Unsound" in Rust terms)

[tustvold]

Describe the bug

It was brought to my attention in apache/arrow-rs#6616 that comet is currently violating the aliasing rules of the Rust compiler. In particular it is mutating memory without exclusive ownership.

The docs on CometBuffer actually call out that the type is unsound - https://github.com/apache/datafusion-comet/blob/main/native/core/src/common/buffer.rs#L166.

This is the underlying cause of #1030, which is a relatively harmless manifestation of what is ultimately undefined behaviour.

Even putting aside that UB effectively means the program could do literally anything, the exact scenario in #1030 could easily lead to out of bounds memory access, e.g. by unmasking a dictionary key that was previously null and now points to some arbitrary memory location.

I debated filing this ticket, as I wasn't sure how it would be received, but I think it is a sufficiently critical vulnerability that should at the very least be tracked / documented. The way it was being dismissed made me honestly a little uncomfortable. Ultimately CometBuffer is unsound, and there is a concrete example of this unsoundness leading to undefined behaviour in #1030.

Steps to reproduce

Expected behavior

No response

Additional context

FYI @viirya

[viirya]

I don't know why this is tagged as bug...

[tustvold]

I don't know why this is tagged as bug...

Apologies, I presumed that a security vulnerability was a bug...

[viirya]

I debated filing this ticket, as I wasn't sure how it would be received, but I think it is a sufficiently critical vulnerability that should at the very least be tracked / documented. The way it was being dismissed made me honestly a little uncomfortable.

As you mentioned the doc of CometBuffer already documents the unsafe behavior. Not sure why you also said it is not documented.

I also don't know why it was being dismissed. We have dealt with that with deep copying the arrays in the necessary cases. The fact is that the take kernel has the inconsistent behavior not well documented which can easily confuse users. Before digging into its detail, how does an user know it clones a buffer sometimes and not sometimes? So for an user, isn't the first impression that it works well when we don't deep copy the array? How does an user know it has different behavior that we must deep copy for it in corner cases? Especially it is not documented.

[alamb]

I tried to update the title of this PR to better reflect what it is reporting.

[tustvold]

I am trying to report that your codebase has a critical bug in its handling of memory, without causing unnecessary angst. Modifying buffers behind the back of the Rust compiler is undefined behaviour, and can trivially lead to out of bounds memory access... This is typically considered a serious security defect...

As you mentioned the doc of CometBuffer already documents the unsafe behavior

CometBuffer does document that it has unsafe invariants, this isn't in and of itself an issue. Yes, it is unsound which Rust purists will complain about, but provided nothing uses it in a way that triggers undefined behaviour it isn't the end of the world. The problem is #1030 highlights that it is not being used in a manner that avoids undefined behaviour. This is what I think should be documented, highlighted, and arguably fixed.

Edit: to expand on this, if, for example, comet were instead using into_mutable().unwrap() or similar, #1030 would instead be a runtime panic, which would avoid UB and make tracking down where assumptions are violated much easier.

[viirya]

The problem is #1030 highlights that it is not being used in a manner that avoids undefined behaviour

As I mentioned earlier, we did effort to avoid it for the cases we know...
But the problem is, nobody knows the kernel behaves inconsistently except for ones already know the details. If we know that in advance, we definitely avoid it like we did for other operations. As I mentioned many times, the kernel behavior is inconsistent and undocumented. It definitely causes user confusion and spend time digging into implementation details just for figuring out why it happens.

[viirya]

Edit: to expand on this, if, for example, comet were instead using into_mutable().unwrap() or similar, #1030 would instead be a runtime panic, which would avoid UB and make tracking down where assumptions are violated much easier.

I believe that Comet scan is developed long before the mutable API comes out...
Not to mention that if it is suitable for our cases.

[alamb]

I believe that Comet scan is developed long before the mutable API comes out.

I wonder if there might be be some way to use Arc / ref counting to verify at runtime that there are no remaining references to the underlying CometBuffer before it is reused 🤔 Kind of like the try_unary_mut style kernel -- that way it would at least be easier to track down places where the assumptions are invalidated (that there is no more sharing)

[viirya]

I wonder if there might be be some way to use Arc / ref counting to verify at runtime that there are no remaining references to the underlying CometBuffer before it is reused

The scan code is not developed by me. I guess that may not work as the CometBuffer internally doesn't use pointer like Arc. It is very low-level raw pointer manipulation. That's why I said the into_mutable may not be suitable.

[tustvold]

As I mentioned earlier, we did effort to avoid it for the cases we know...
But the problem is, nobody knows the kernel behaves inconsistently except for ones already know the details.

So basically it is safe predicated on undocumented assumptions about third-party code. This is at best optimistic 😅

I dunno, I'm going to disengage at this point but I have to say I have found the response here deeply disappointing and tbh rather troubling. I had hoped there'd at least be an acknowledgement that this API is hard to use safely, but instead the response is to suggest the problem lies in the third-party code for not conforming to your assumptions, and that you'll just add some extra copies to "workaround" this... There are ways to do this safely and I would have hoped that would be seen as a north star, even if not a priority at the moment...

[alamb]

Well, I don't presume to know what is / isn't the right design for other systems.

I think this ticket will serve as a good discussion for how to potentially improve the situation and I don't find the discussions or responses troubling.

[alamb]

The scan code is not developed by me. I guess that may not work as the CometBuffer internally doesn't use pointer like Arc. It is very low-level raw pointer manipulation. That's why I said the into_mutable may not be suitable.

Makes sense -- thus it sounds like removing this assumption is likely a non trivial effort

What I personally hope / plan to do is to is work with people make the ParquetExec reader in DataFusion so utterly compelling in terms of performance that Comet can consider using that instead of CometBuffer.

Current obsession is making pushdown predicates even faster with @XiangpengHao and @tustvold: apache/arrow-rs#5523