bit2swaz commented on issue #10679: URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5342852424
> it does seem a bit clunky, and has a downside in that it technically keeps the old metadata around fair, thats a real downside of C. A is cleaner and im happy to go with it, the 60.0.0 breaking window makes it a good time. one thing to watch with A tho is that making it `unsafe` fixes the foreign-schema case but it doesnt close #10286 on its own https://github.com/apache/arrow-rs/blob/505afcbfd2c27897f747916e782d41e1593b7007/arrow-schema/src/ffi.rs#L280-L281 `empty()` leaves `private_data` null, and the `Box::from_raw` at L246 https://github.com/apache/arrow-rs/blob/505afcbfd2c27897f747916e782d41e1593b7007/arrow-schema/src/ffi.rs#L246 runs unconditionally, so `empty().with_metadata(...)` is still UB even for a caller doing everything right on an arrow-rs schema. no safety contract can rule that out since `empty()` is our own constructor so A needs a null guard on `private_data` too, then it closes both. want me to fold #10286 into the same PR, or should i keep it separate? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
