Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rust Push::alignment Incorrect for Structs #8150

Open
tustvold opened this issue Nov 8, 2023 · 5 comments · May be fixed by #8398
Open

Rust Push::alignment Incorrect for Structs #8150

tustvold opened this issue Nov 8, 2023 · 5 comments · May be fixed by #8398

Comments

@tustvold
Copy link
Contributor

tustvold commented Nov 8, 2023

Consider a struct of the form

struct FieldNode {
  length: long;
  null_count: long;
}

The following code is generated for Push

impl<'b> flatbuffers::Push for FieldNode {
    type Output = FieldNode;
    #[inline]
    unsafe fn push(&self, dst: &mut [u8], _written_len: usize) {
        let src =
            ::core::slice::from_raw_parts(self as *const FieldNode as *const u8, Self::size());
        dst.copy_from_slice(src);
    }
}

This therefore uses the default impl of Push::alignment which is

fn alignment() -> PushAlignment {
    PushAlignment::new(align_of::<Self::Output>())
}

Unfortunately the definition of FieldNode is

pub struct FieldNode(pub [u8; 16]);

Which has an alignment of 1.

The net result is that the writer does not provide the correct alignment guarantees for structs, which causes the verifiers of some implementations to fail - apache/arrow-rs#5052.
@tustvold tustvold mentioned this issue Nov 8, 2023
Open
@tustvold
Copy link
Contributor Author

@CasperN perhaps you have some thoughts on this?

@evgenyx00
Copy link

@tustvold btw, meanwhile based on your findings, I did a "hack" in flatbuffer create_vector/create_vector_from_iter and changed the alignment to 8.

@jpochyla
Copy link

So the codegen in the official Rust backend is generating code that produces wrong data (probably coming from misunderstanding of Rust alignment rules) and none of the maintainers care?

@github-actions GitHub Actions
Copy link

This issue is stale because it has been open 6 months with no activity. Please comment or label not-stale, or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Jun 26, 2024
@evgenyx00
Copy link

This is still an issue, adding a comment to avoid closure of this problem.

@github-actions github-actions bot removed the stale label Jul 25, 2024
This was referenced Sep 19, 2024
Open
Open
Open
@paleolimbot paleolimbot mentioned this issue Sep 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

use ALIGN for Push::alignment in struct types bkietz/flatbuffers
3 participants
@jpochyla @tustvold @evgenyx00