linebender / piet

An abstraction for 2D graphics.
Apache License 2.0
1.23k stars 94 forks source link

Input validation for bitmap_target #384

Open johnmave126 opened 3 years ago

johnmave126 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

I borrowed a mac and verified that https://github.com/linebender/piet/blob/2cfa269fea092c1a528cf7b3ba97cefbbbe9e288/piet-common/src/cg_back.rs#L78-L86 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 commented 3 years ago

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