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.

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

Input validation for bitmap_target #384

Open
johnmave126 opened this issue Jan 2, 2021 · 3 comments
Open

Input validation for bitmap_target #384

johnmave126 opened this issue Jan 2, 2021 · 3 comments
Labels
enhancement New feature or request

Comments

@johnmave126
Copy link

Currently there is no validation for the input arguments to bitmap_target. Given that the underlying call unwraps instead of propagating error, the program could panic when input is invalid.

For example
https://github.com/linebender/piet/blob/master/piet-common/src/direct2d_back.rs#L108-L111
https://github.com/linebender/piet/blob/master/piet-common/src/cairo_back.rs#L81

The program will panic if width == 0 and/or height == 0. I am wondering whether it makes sense to validate input arguments and return Error::InvalidInput if the validation fails before sending the request to backend.

@cmyr cmyr added the enhancement New feature or request label Jan 3, 2021
@cmyr
Copy link
Member

cmyr commented Jan 3, 2021

I agree it makes sense to have this return a Result; either we could validate ourselves or just wrap the inner error instead of unwrapping; I would prefer the latter unless there's a situation where we segfault (which macOS likes to do sometimes when you pass bad args) in which case validation would totally make sense.

@johnmave126
Copy link
Author

I borrowed a mac and verified that

let ctx = CGContext::create_bitmap_context(
None,
width,
height,
8,
0,
&CGColorSpace::create_device_rgb(),
core_graphics::base::kCGImageAlphaPremultipliedLast,
);

will panic if width and/or height are 0. Hence I guess input validation, at least for mac, is needed.

Apart from that, should pix_scale be verified? I tried passing 0.0 and -1.0 and the call succeeded with no panic/error but I suspect there will be problem in later usage.

@cmyr
Copy link
Member

cmyr commented Jan 10, 2021

I think that an validation that improves correctness is good, especially in situations where we are already returning a Result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants