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

Cleanup/improve safety: remove goto and potentially unbounded for loop #120

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 28 additions & 43 deletions encoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ type encMode uint
const (
// a qrbool of EncModeAuto will trigger a detection of the letter set from the input data,
EncModeAuto = 0
// EncModeNone mode ...
// EncModeNone mode is unused.
EncModeNone encMode = 1 << iota
// EncModeNumeric mode ...
// EncModeNumeric mode uses numeric characters.
EncModeNumeric
// EncModeAlphanumeric mode ...
// EncModeAlphanumeric mode uses alpha numeric characters.
EncModeAlphanumeric
// EncModeByte mode ...
// EncModeByte mode byte.
EncModeByte
// EncModeJP mode ...
// EncModeJP mode japan.
EncModeJP
)

Expand Down Expand Up @@ -80,6 +80,7 @@ type encoder struct {
version version // QR version ref
}

// newEncoder creates a new encoder.
func newEncoder(m encMode, ec ecLevel, v version) *encoder {
return &encoder{
dst: nil,
Expand All @@ -93,7 +94,6 @@ func newEncoder(m encMode, ec ecLevel, v version) *encoder {
// Encode ...
// 1. encode raw data into bitset
// 2. append _defaultPadding data
//
func (e *encoder) Encode(byts []byte) (*binary.Binary, error) {
e.dst = binary.New()
e.data = byts
Expand Down Expand Up @@ -214,6 +214,7 @@ func (e *encoder) breakUpInto8bit() {
}
}

// charCountMap is a map of character encodings to bit counts.
// 字符计数指示符位长字典
var charCountMap = map[string]int{
"9_numeric": 10,
Expand All @@ -230,7 +231,7 @@ var charCountMap = map[string]int{
"40_japan": 12,
}

// charCountBits
// charCountBits returns the number of bits for the character count.
func (e *encoder) charCountBits() int {
var lv int
if v := e.version.Ver; v <= 9 {
Expand Down Expand Up @@ -281,10 +282,6 @@ func encodeAlphanumericCharacter(v byte) uint32 {
return 0
}

// analyzeEncFunc returns true is current byte matched in current mode,
// otherwise means you should use a bigger character set to check.
type analyzeEncFunc func(byte) bool

// analyzeEncodeModeFromRaw try to detect letter set of input data,
// so that encoder can determine which mode should be use.
// reference: https://en.wikipedia.org/wiki/QR_code
Expand All @@ -293,47 +290,40 @@ type analyzeEncFunc func(byte) bool
// case2: could not use EncModeNumeric, but you can find all of them in character mapping, use EncModeAlphanumeric.
// case3: could not use EncModeAlphanumeric, but you can find all of them in ISO-8859-1 character set, use EncModeByte.
// case4: could not use EncModeByte, use EncModeJP, no more choice.
//
func analyzeEncodeModeFromRaw(raw []byte) encMode {
analyzeFnMapping := map[encMode]analyzeEncFunc{
EncModeNumeric: analyzeNum,
EncModeAlphanumeric: analyzeAlphaNum,
EncModeByte: nil,
EncModeJP: nil,
}

var (
f analyzeEncFunc
mode = EncModeNumeric
)

// loop to check each character in raw data,
// from low mode to higher while current mode could bearing the input data.
mode := EncModeNumeric
for _, byt := range raw {
reAnalyze:
if f = analyzeFnMapping[mode]; f == nil {
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)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any meaning of converting a byte to a rune? raw is a byte slice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

continue
}

// issue#28 @borislavone reports this bug.
// FIXED(@yeqown): next encMode analyzeVersionAuto func did not check the previous byte,
// add goto statement to reanalyze previous byte which can't be analyzed in last encMode.
if !f(byt) {
mode <<= 1
goto reAnalyze
if analyzeAlphaNum(rune(byt)) {
mode = EncModeAlphanumeric
} else {
// If it's nothing else then it's raw byte mode
// TODO if it's outside of ISO-8859-1 then use [EncModeJP].
// It is hard to detect ISO-8859-1 though since it includes so many things.
// Attempt to detect japenese instead in the future.
return EncModeByte
}

// TODO add analyzers for other modes
// EncModeJP
// Do not need to explicitly check for byte since that is just the fallback mode.
}

return mode

}

// analyzeNum is byt in num encMode
func analyzeNum(byt byte) bool {
func analyzeNum(byt rune) bool {
return byt >= '0' && byt <= '9'
}

// analyzeAlphaNum is byt in alpha number
func analyzeAlphaNum(byt byte) bool {
func analyzeAlphaNum(byt rune) bool {
if (byt >= '0' && byt <= '9') || (byt >= 'A' && byt <= 'Z') {
return true
}
Expand All @@ -343,8 +333,3 @@ func analyzeAlphaNum(byt byte) bool {
}
return false
}

//// analyzeByte is byt in bytes.
//func analyzeByte(byt byte) qrbool {
// return false
//}
32 changes: 16 additions & 16 deletions encoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func TestEncodeByte(t *testing.T) {

func Test_analyzeNum(t *testing.T) {
type args struct {
byt byte
input rune
}
tests := []struct {
name string
Expand All @@ -64,33 +64,33 @@ func Test_analyzeNum(t *testing.T) {
}{
{
name: "case 0",
args: args{byt: '0'},
args: args{input: '0'},
want: true,
},
{
name: "case 1",
args: args{byt: 'a'},
args: args{input: 'a'},
want: false,
},
{
name: "case 2",
args: args{byt: 'A'},
args: args{input: 'A'},
want: false,
},
{
name: "case 3",
args: args{byt: '9'},
args: args{input: '9'},
want: true,
},
{
name: "case 4",
args: args{byt: '*'},
args: args{input: '*'},
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := analyzeNum(tt.args.byt); got != tt.want {
if got := analyzeNum(tt.args.input); got != tt.want {
t.Errorf("analyzeNum() = %v, want %v", got, tt.want)
}
})
Expand All @@ -99,7 +99,7 @@ func Test_analyzeNum(t *testing.T) {

func Test_analyzeAlphanum(t *testing.T) {
type args struct {
byt byte
input rune
}
tests := []struct {
name string
Expand All @@ -108,43 +108,43 @@ func Test_analyzeAlphanum(t *testing.T) {
}{
{
name: "case 0",
args: args{byt: '0'},
args: args{input: '0'},
want: true,
},
{
name: "case 1",
args: args{byt: 'a'},
args: args{input: 'a'},
want: false,
},
{
name: "case 2",
args: args{byt: 'A'},
args: args{input: 'A'},
want: true,
},
{
name: "case 3",
args: args{byt: '9'},
args: args{input: '9'},
want: true,
},
{
name: "case 4",
args: args{byt: '*'},
args: args{input: '*'},
want: true,
},
{
name: "case 5",
args: args{byt: '?'},
args: args{input: '?'},
want: false,
},
{
name: "case 6",
args: args{byt: '&'},
args: args{input: '&'},
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := analyzeAlphaNum(tt.args.byt); got != tt.want {
if got := analyzeAlphaNum(tt.args.input); got != tt.want {
t.Errorf("analyzeAlphaNum() = %v, want %v", got, tt.want)
}
})
Expand Down
8 changes: 3 additions & 5 deletions qrcode.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func (q *QRCode) init() (err error) {
return fmt.Errorf("init: calc version failed: %v", err)
}
q.mat = newMatrix(q.v.Dimension(), q.v.Dimension())
_ = q.applyEncoder()
q.applyEncoder()

var (
dataBlocks []dataBlock // data encoding blocks
Expand Down Expand Up @@ -153,11 +153,9 @@ func (q *QRCode) calcVersion() (ver *version, err error) {
return
}

// applyEncoder
func (q *QRCode) applyEncoder() error {
// applyEncoder sets the encoder based on encoding mode, error correction level, and version.
func (q *QRCode) applyEncoder() {
q.encoder = newEncoder(q.encodingOption.EncMode, q.encodingOption.EcLevel, q.v)

return nil
}

// dataEncoding ref to:
Expand Down
5 changes: 3 additions & 2 deletions writer/standard/writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Owner

Choose a reason for hiding this comment

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

I couldn't agree that goto is unsafe here, replacing goto by directly return is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

// DONE(@yeqown): calculate the xOffset and yOffset which point(xOffset, yOffset)
// should icon upper-left to start
dc.DrawImage(opt.logoImage(), (w-logoWidth)/2, (h-logoHeight)/2)
}
done:

return dc.Image()
}

Expand Down