Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

linuxSeccompBase: Fix errno check #692

Merged
merged 1 commit into from
Sep 19, 2017
Merged

linuxSeccompBase: Fix errno check #692

merged 1 commit into from
Sep 19, 2017

Conversation

glevand
Copy link
Contributor

@glevand glevand commented Sep 6, 2017

errno symbols can contain numeric characters.

@alban
Copy link
Member

alban commented Sep 8, 2017

LGTM

Examples:

$ errno --list|gawk '{print $1}'|grep [0-9]
E2BIG
EL2NSYNC
EL3HLT
EL3RST
EL2HLT

@@ -227,7 +227,7 @@ func (l linuxSeccompBase) AssertValid() error {
return nil
}
for _, c := range l.val.Errno {
if !unicode.IsUpper(c) {
if !(unicode.IsUpper(c) || unicode.IsNumber(c)) {
return errors.New("errno must be an upper case string")
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message could be also updated there...
E.g. "errno must be a string containing only digits and upper case characters"

Copy link
Contributor

Choose a reason for hiding this comment

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

As we wish, I think the additional OR-condition doesn't change the spirit of the previous error label. @jellonek's suggestion is strictly speaking more correct.

@lucab
Copy link
Contributor

lucab commented Sep 14, 2017

Definitely my bad, I forgot some of them contain numbers. Perhaps a better check would be starts with uppercase "E" and contains only uppercase or digits.

@glevand
Copy link
Contributor Author

glevand commented Sep 18, 2017

Updated patch to do the checks as suggested.

cc: @lucab

@lucab
Copy link
Contributor

lucab commented Sep 18, 2017

(go fmt is unhappy about the latest rebase)

errno must start with an 'E' character, and can also contain
numeric characters.  Fixup the errno check for these.

Also, improve the error message by printing the offending errno
value.

Signed-off-by: Geoff Levand <geoff@infradead.org>
@glevand
Copy link
Contributor Author

glevand commented Sep 18, 2017

Fixed gofmt.

Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

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

LGTM

@squeed squeed merged commit d6679c6 into appc:master Sep 19, 2017
@glevand glevand deleted the for-merge-linuxSeccompBase branch September 19, 2017 16:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants