Allow c_data() to return zero byte buffer

[kevinbackhouse]

This reverts a change that was made in #2209. I argued at the time that it was a bad idea. Since then it has caused at least two bugs that I'm aware of: #2565, #2650.

[ghost]

👇 Click on the image for a new way to code review

Legend

[kmilos]

It'll still return an address not owned by the DataBuf, no?

[kevinbackhouse]

It'll still return an address not owned by the DataBuf, no?

It's the top edge of the buffer, so it's fine as long as you read zero bytes. That's quite a common edge case, so I think it should be allowed.

[codecov]

Codecov Report

Merging #2654 (5f9ecad) into main (b66ebd9) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #2654      +/-   ##
==========================================
- Coverage   63.92%   63.92%   -0.01%     
==========================================
  Files         103      103              
  Lines       22306    22306              
  Branches    10795    10795              
==========================================
- Hits        14260    14259       -1     
  Misses       5826     5826              
- Partials     2220     2221       +1     

Impacted Files	Coverage Δ
src/types.cpp	95.53% <50.00%> (-0.28%)	⬇️

[kmilos]

Isn't it a potential exploit? That we would provide on our public API?

[kevinbackhouse]

Isn't it a potential exploit? That we would provide on our public API?

I don't think so. The old code only protected against out-of-bounds reads that looked like this:

memcpy(mybuf, buf.c_data(buf.size()), 10);

But it didn't protect against this, which is almost exactly the same thing:

memcpy(mybuf, buf.c_data(buf.size() - 1), 11);

[kevinbackhouse]

That fuzzer failure is concerning. It probably means that this code was masking a bug somewhere else. I'll investigate.

[kmilos]

I wasn't thinking about our code.

[kevinbackhouse]

I'm sorry, I made a mistake. It turns out there's a similar assertion in std::vector::operator[], which is what's causing the fuzzing failure. I've added a third commit which returns nullptr in this edge case. I think that will work well because it stops this code from throwing an exception in the edge case where the client is reading zero bytes at the end of the buffer, but it will throw a null pointer exception if they actually try to read the out-of-bounds bytes.

[neheb]

@caclark this fix things?

[caclark]

@neheb No, unfortunately this does not fix the Geeqie related bug.

[neheb]

@Mergifyio backport 0.28.x

[mergify]

backport 0.28.x

✅ Backports have been created

• #2655 Allow c_data() to return zero byte buffer (backport #2654) has been created for branch 0.28.x