That looks great. I could turn on the big bad rev.dep machine but I am not even sure it's needed.

Are we sure std::copy() is going to accept a null pointer here? If we don't already have it, a unit test would be nice -- presumably ASAN / UBSAN builds of Rcpp with those tests would catch this issue.

I'm also fairly certain the actual issue here comes from this bit of cruel code.

https://github.com/wch/r-source/blob/6b5f72eebd9211766bd159e017a167385c93772e/src/include/Rinlinedfuns.h#L135-L142

Perhaps we can return dataptr(x) instead of NULL? With the appropriate casting.

Or, alternatively -- instead of using __ACCESSOR__ here, just use DATAPTR_RO, and cast to the appropriate type. But we're sort of cheating since DATAPTR_RO is supposed to only give read access to memory, and these might be writable...

I just hate the idea of this zero-length check becoming necessary. :|

dataptr has the same problem, and I think we should add the same workaround, because all these accessors end up calling that macro I linked above, which doesn't check the length and therefore return an invalid pointer when the vector is empty.

std::copy works fine with a null pointer, I tested it with the reprex provided by the update, but of course I can add a test.

Note also that the vector cache sets start to zero upon initialization, but this is messed up when the update method is called. So this follows the initial idea of having a null pointer as the start of there is no start.

dataptr has the same problem, and I think we should add the same workaround, because all these accessors end up calling that macro I linked above, which doesn't check the length and therefore return an invalid pointer when the vector is empty.

Are you sure? dataptr goes through R's DATAPTR_RO, which shouldn't have the same 0x1 path for empty vectors. Please correct me if I'm wrong.

https://github.com/wch/r-source/blob/6b5f72eebd9211766bd159e017a167385c93772e/src/include/Rinlinedfuns.h#L148-L154

https://github.com/wch/r-source/blob/6b5f72eebd9211766bd159e017a167385c93772e/src/include/Defn.h#L421

The last one is STDVEC_DATAPTR, which is what I linked above, and has this +1 unconditionally. Or am I missing something?

Oh, I get what you mean, the other macro will return a valid address +1. Is that a valid memory address anyway? In the sense, it may be unallocated.

In fact, DATAPTR was avoided in #1310 in response to #1308. :-\

Oh, I get what you mean, the other macro will return a valid address +1. Is that a valid memory address anyway? In the sense, it may be unallocated.

Good question. I think the C++ standard guarantees that pointers to the "end" of an allocation are still valid, as long as you don't de-reference those pointers. I don't know if that extends to memory allocated from C, though...

Honestly though, the right solution might just be for us to request R Core back out this change upstream.

Ok, let's go back to dataptr() then without workarounds. The original test case #1461 works without UB. And I've run the entire test suite and there are no issues. (In fact, I've found another UB, but unrelated; another fight for another day).

Thanks @Enchufa2; sorry for being so nit-picky here!

That's fine, that's the purpose of a review!