Cleanup/improve safety: remove goto and potentially unbounded for loop #120
Conversation
- reduce usage of goto to simplify code - remove unused and unchecked error return value
- removes usage of goto and potentially neverending loop which could lead to DOS attack - reduce requirement for fuzzing
epelc
changed the title
| break | ||
| // Start from most inclusive and work backwards | ||
| // Check for each kind starting with most common then expand out to least inclusive. | ||
| if analyzeNum(rune(byt)) { |
There was a problem hiding this comment.
Is there any meaning of converting a byte to a rune? raw is a byte slice
There was a problem hiding this comment.
Characters are runes in go. Using rune instead of byte encourages comparing to a single character which is the point of these functions since they aren’t Unicode aware.
| @@ -193,14 +193,15 @@ func draw(mat qrcode.Matrix, opt *outputImageOptions) image.Image { | |||
| if !validLogoImage(w, h, logoWidth, logoHeight, opt.logoSizeMultiplier) { | |||
| log.Printf("w=%d, h=%d, logoW=%d, logoH=%d, logo is over than 1/%d of QRCode \n", | |||
| w, h, logoWidth, logoHeight, opt.logoSizeMultiplier) | |||
| goto done | |||
|
|
|||
| return dc.Image() | |||
There was a problem hiding this comment.
I couldn't agree that goto is unsafe here, replacing goto by directly return is not necessary
There was a problem hiding this comment.
It’s one less goto to have to check for security issues. Usually they are dangerous during for loops as you can easily end up with unbounded loops or other issues especially if you aren’t fuzzing.
In this case it’s simple but I think just adding the return here or switching this to an if else on the ‘dc.DrawImage’ is an easy way to avoid it and thus make security reviews for new users have less red flags to check.
This pr contains several fixes but primarily aims to make
analyzeEncodeModeFromRawsafer by removing the use of a goto statement along with complex boolean checking. Aim is to make this library safer to use in production with web services.Also includes cleanups to further document some methods/variables and remove unused error return value.