[C++][FlightRPC] Flight generates misaligned buffers

[asfimport]

Protobuf's wire format design + our zero-copy serializer/deserializer mean that buffers can end up misaligned. On some Arrow versions, this can cause segfaults in kernels assuming alignment (and generally violates expectations).

We should:

• Possibly include buffer alignment in array validation

• See if we can adjust the serializer to somehow pad things properly

• See if we can do anything about this in the deserializer

  Example:

  import pyarrow as pa
  import pyarrow.flight as flight
  
  class TestServer(flight.FlightServerBase):
      def do_get(self, context, ticket):
          schema = pa.schema(
              [
                  ("index", pa.int64()),
                  ("int8", pa.float64()),
                  ("int16", pa.float64()),
                  ("int32", pa.float64()),
              ]
          )
          return flight.RecordBatchStream(pa.table([
              [0, 1, 2, 3],
              [0, 1, None, 3],
              [0, 1, 2, None],
              [0, None, 2, 3],
          ], schema=schema))
  
  
  with TestServer() as server:
      client = flight.connect(f"grpc://localhost:{server.port}")
      table = client.do_get(flight.Ticket(b"")).read_all()
      for col in table:
          print(col.type)
          for chunk in col.chunks:
              for buf in chunk.buffers():
                  if not buf: continue
                  print("buffer is 8-byte aligned?", buf.address % 8)
              chunk.cast(pa.float32())

  On Arrow 8

  
  int64
  buffer is 8-byte aligned? 1
  double
  buffer is 8-byte aligned? 1
  buffer is 8-byte aligned? 1
  double
  buffer is 8-byte aligned? 1
  buffer is 8-byte aligned? 1
  double
  buffer is 8-byte aligned? 1
  buffer is 8-byte aligned? 1
  

  On Arrow 7

  
  int64
  buffer is 8-byte aligned? 4
  double
  buffer is 8-byte aligned? 4
  buffer is 8-byte aligned? 4
  fish: Job 1, 'python ../test.py' terminated by signal SIGSEGV (Address boundary error)
  

Reporter: David Li / @lidavidm

_{Note: This issue was originally created as ARROW-16958. Please see the migration documentation for further details.}

[asfimport]

Antoine Pitrou / @pitrou:
Ouch, our kernels should not assume alignment. What kind of instructions are generated that fail on misaligned data?

[asfimport]

David Li / @lidavidm:
I didn't look too closely since it was fixed in Arrow 8.0.0, I just haven't gotten to look yet whether it was an intentional or unintentional fix

[asfimport]

David Li / @lidavidm:
FWIW in Arrow 7 it's this sequence

   │0x7ffff56f06b7 <_ZN5arrow7compute8internal12_GLOBAL__N_112DoStaticCastIfdEEvPKvlllPv+119>       shr    $0x2,%r9                                                                   │
   │0x7ffff56f06bb <_ZN5arrow7compute8internal12_GLOBAL__N_112DoStaticCastIfdEEvPKvlllPv+123>       nopl   0x0(%rax,%rax,1)                                                           │
  >│0x7ffff56f06c0 <_ZN5arrow7compute8internal12_GLOBAL__N_112DoStaticCastIfdEEvPKvlllPv+128>       cvtpd2ps (%rsi,%rax,2),%xmm0                                                      │
   │0x7ffff56f06c5 <_ZN5arrow7compute8internal12_GLOBAL__N_112DoStaticCastIfdEEvPKvlllPv+133>       cvtpd2ps 0x10(%rsi,%rax,2),%xmm1                                                  │
   │0x7ffff56f06cb <_ZN5arrow7compute8internal12_GLOBAL__N_112DoStaticCastIfdEEvPKvlllPv+139>       add    $0x1,%rcx    

so an SSE instruction. Backtrace is

#0  0x00007ffff56f06c0 in void arrow::compute::internal::(anonymous namespace)::DoStaticCast<float, double>(void const*, long, long, long, void*) ()
   from /home/lidavidm/miniconda3/envs/theseus_env/lib/python3.9/site-packages/pyarrow/../../../libarrow.so.700
#1  0x00007ffff56fc17f in arrow::compute::internal::CastNumberToNumberUnsafe(arrow::Type::type, arrow::Type::type, arrow::Datum const&, arrow::Datum*) ()
   from /home/lidavidm/miniconda3/envs/theseus_env/lib/python3.9/site-packages/pyarrow/../../../libarrow.so.700
#2  0x00007ffff5709c73 in arrow::compute::internal::CastFloatingToFloating(arrow::compute::KernelContext*, arrow::compute::ExecBatch const&, arrow::Datum*) ()
   from /home/lidavidm/miniconda3/envs/theseus_env/lib/python3.9/site-packages/pyarrow/../../../libarrow.so.700 

[asfimport]

David Li / @lidavidm:
Hmm, I wonder if I misdiagnosed…I thought it would be alignment, but GDB says the code dereferenced a null pointer.

[asfimport]

Yifei Yang:
Hey, I guess I also got the similar issue. What I did is to use Flight to transfer an Arrow table and then feed it into Arrow's aggregate exec node to do hash-aggregate. It will crash at arrow::util::CheckAlignment(). However using the original table works well. For the transferred table, if I first serialize into bytes then recreate an arrow table using the bytes, it also works well, which I guess is because the newly created table from bytes is aligned. I tested on both 6.0.0 and 8.0.0.

[asfimport]

Antoine Pitrou / @pitrou:
[~yyfyyf123123] Can you open a JIRA for the crash at arrow::util::CheckAlignment() ? I don't think it should crash.

[asfimport]

Yifei Yang:
@pitrou Ticket (https://issues.apache.org/jira/browse/ARROW-17783) here, I'm still trying to reproduce it stably in a minimal test.

[asfimport]

Yifei Yang:
@pitrou I'm now able to reproduce the issue stably, and I attached the source into that ticket.

[asfimport]

Weston Pace / @westonpace:
I can look into CheckAlignment but I still think it would be nice to fix this in flight. Otherwise data received from flight that is then passed over the C data interface could fail if the receiving library doesn't support unaligned buffers. @marsupialtail just mentioned to me that they ran into this kind of situation trying to pass a buffer to polars (I believe arrow-rs expects buffers of T to be aligned to T).

[asfimport]

David Li / @lidavidm:
Unless I come up with a great idea, that's tantamount to forcing a copy on all data coming through flight - which may not be awful given that gRPC often forces a copy on us (right now), but still isn't ideal

[asfimport]

David Li / @lidavidm:
I've sort of wanted to explore this anyways in any case - can we match up the chunks that gRPC gives us to the buffers in the IPC payload, and avoid concatenating? I suppose that optimization would bring little benefit if we ended up having to align all of them anyways, though. And as mentioned, it might be possible to insert padding data to align the buffer on the wire, but then you can't count on the client being polite like that.

[asfimport]

Apache Arrow JIRA Bot:
This issue was last updated over 90 days ago, which may be an indication it is no longer being actively worked. To better reflect the current state, the issue is being unassigned per project policy. Please feel free to re-take assignment of the issue if it is being actively worked, or if you plan to start that work soon.

[tustvold]

A number of users are running into this, perhaps in lieu of a proper solution we might be able to provide a workaround? Perhaps a kernel that properly aligns input buffers? Perhaps such a kernel already exists?

Whilst we could patch around this in arrow-rs FFI it undermines the zero-copy expectation of FFI, and of course doesn't help with all the other arrow implementations that require alignment.

[westonpace]

@tustvold

We recently added such a utility #14758 and also recently started using it in the Acero source node since Acero relies on alignment.

[westonpace]

Although the solution in #14758 is not zero-copy if the buffers are unaligned.

[alexander-beedie]

Hi all, just following up on this - I see there are various workarounds being implemented in client/downstream code, but is it likely that the alignment issue is going to be addressed, or should we assume that receiving unaligned buffers from Flight is normal/expected and handle accordingly?

If so, perhaps the spec could be clarified - my original understanding from reading it was that Flight/IPC data is always supposed to be aligned (whereas Arrow recommends alignment), but if that's wrong then please correct me ;)

We've been trying to use Flight internally (at work), feeding the resulting data into Polars (which I contribute to, and which uses Rust's arrow2 implementation), but we cannot as it consistently results in errors along the lines of OutOfSpec "An ArrowArray of type <xyz> must have buffer 1 aligned to type i64"¹.

Thanks!

Footnotes

1. Error raised here: src/ffi/array.rs#L194 ↩

[RichardHaythorn]

IIRC, its due to gRPC or protobuf that there's never a guarantee of Flight data having the buffers aligned.

(I have had issues regarding the misaligned buffers for months at work, see pola-rs/polars#6315. I haven't found any zero-copy solutions yet.)

[lidavidm]

#35679 was the fix, but it was not accepted.

There is no zero-copy solution because of Protobuf indeed, but gRPC effectively forces a copy on you anyways in C++; it may be possible to do one copy instead of two, but I don't believe you can get to zero-copy except in unusual circumstances. It may be possible to have servers pad the Protobuf message in a way to align the data, but that would also not be a universal fix (and would not be backwards compatible, since some implementations unfortunately reject unknown field tags instead of ignoring them as they should)

[lidavidm]

I'll see if I can find the time to take a second look, given that I'm digging around here again. At the very least, I want to fix C++ rejecting unknown tags (#36975)

[alexander-beedie]

Thanks all; if a general fix becomes available that would be ideal, otherwise looks like it'll probably require an extra copy to get it working.

[orlp]

@alexander-beedie Arrow only recommends alignment for internal processes, it requires it for IPC.

Implementations are recommended to allocate memory on aligned addresses (multiple of 8- or 64-bytes) and pad (overallocate) to a length that is a multiple of 8 or 64 bytes. When serializing Arrow data for interprocess communication, these alignment and padding requirements are enforced.

If Flight is handing out misaligned buffers to IPC it is non-compliant and must be fixed.

[alexander-beedie]

Arrow only recommends alignment for internal processes, it requires it for IPC.

That was also my understanding. Some clarification of the docs, the spec, or a fix would be great, otherwise all downstream consumers expecting aligned data are going to discover (and have to solve) the issue individually.

[jacques-n]

It was never the intention that flight data on the receiving side must be aligned. The intention some of us always had was if you need something aligned and it isn't you pay the copy then, not before. It'd be great for individual flight clients to have an option to ensure aligned with clear messaging about this possibly incurring a copy but forcing flight to always be aligned because some people rely on alignment just punishes those that don't. If presented as an option, this allows the individual client implementations to choose to optimize away copies now, later or never.

(As a note the documentation cited above says nothing about the receiving side afaict).

Wrt to unknown tag padding. We could simply add one more dummy field (I don't believe there is any requirement for tags to be in order in messages) or simply restate a field with the first one being arbitrary padding. If I recall correctly, protobuf spec is that the last tag value for a specific tag within a message wins and previous ones should be ignored. (comes from streaming where something can be overridden by being expressed a second time)

[lidavidm]

I still intend to look at the options here when I get a chance. I need to do some refactoring in C++ first to be able to pass down options to the deserializer.

Also, see #34485 where we may want to evaluate a new framing format for Flight to support this and also things like messages that don't fit within a single gRPC/Protobuf message, and #37900 where we may want to evaluate if we can get more control over memory usage from Flight/gRPC.