Skip to content

Conversation

@hasenbanck
Copy link

It is currently possible to trigger UB by reading uninitialized memory using the API, which result for example under Windows in a STATUS_ACCESS_VIOLATION:

use intel_tex_2::RgbaSurface;

fn main() {
    let pixel_size = size_of::<u32>() as u32;
    let width = 64;
    let height = 64;

    let input = vec![0xA1; (width * height * pixel_size) as usize];

    let output_size = intel_tex_2::bc3::calc_output_size(width, height);
    let mut output = vec![0x0; output_size];

    intel_tex_2::bc3::compress_blocks_into(
        &RgbaSurface {
            data: &input,
            width,
            height,
            stride: width * (pixel_size + 2),
        },
        &mut output,
    );
}

This PR makes sure that the data of the surface given actually is big enough for its height and stride.

@MarijnS95 MarijnS95 changed the title Fix unsound API design Assert when input data is too small for RgbaSurface Jan 16, 2025
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, seems we already checked the output buffer but never the input buffer.

Regarding the clippy lint, we have an fn divide_up_by_multiple() in lib.rs that also seems to open-code div_ceil(), feel free to fix both and unblock the CI.

It is currently possible to trigger UB by reading uninitialized memory using the API. This PR makes sure that the data of the surface given actually is big enough for its height and stride.
@hasenbanck
Copy link
Author

  • Renamed the commit message based on the changed title of the PR
  • Fixed lint suggestion
  • Fixed fmt

@MarijnS95
Copy link
Member

MarijnS95 commented Jan 16, 2025

Can you also remove our open-coded divide_up_by_multiple()? Otherwise I'll remove that in a followup.

clippy is probably not pointing out div_ceil() because it's written with a let binding?

@hasenbanck
Copy link
Author

Can you also remove our open-coded divide_up_by_multiple()? Otherwise I'll remove that in a followup.

I didn't include the change in the PR, so I will fix it. Should I remove divide_up_by_multiple() alltogether?

@MarijnS95
Copy link
Member

Yeah I would remove it altogether if it serves no purpose other than wrapping an existing std function.

@hasenbanck
Copy link
Author

Removed all instances of manually implementing div_ceil() and also removed divide_up_by_multiple() too.

@MarijnS95
Copy link
Member

Don't think I can rebase-merge so the resulting commits will be squash-merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants