handleing multiple invalid \ in a model

[jccardonar]

Hello,

I am using goyang to process MIB-YANG modules obtained after converting SMIv2 files with SMIDUMP.

This issue is about handling multiple invalid characters after a backslash () in a YANG module.

To reproduce:

• Clone the modules in repository https://github.com/jccardonar/goyang_reports
• Process the module CISCO-IMAGE-TC.yang with goyang:

goyang CISCO-IMAGE-TC.yang

Output should be:

goyang CISCO-IMAGE-TC.yang
fatal error: all goroutines are asleep - deadlock!
 
goroutine 1 [chan send]:
github.com/openconfig/goyang/pkg/yang.(*lexer).emitText(0xc8200ca7e0, 0xfffffffffffffffe, 0xc8200dcdae, 0xad)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/lex.go:200 +0x2dd
github.com/openconfig/goyang/pkg/yang.(*lexer).emit(0xc8200ca7e0, 0xfffffffffffffffe)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/lex.go:185 +0x66
github.com/openconfig/goyang/pkg/yang.(*lexer).Errorf(0xc8200ca7e0, 0xc8200af0c8, 0x1b, 0x0, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/lex.go:319 +0x5fa
github.com/openconfig/goyang/pkg/yang.(*lexer).ErrorfAt(0xc8200ca7e0, 0x7b, 0x5, 0xc8200af0c8, 0x1b, 0x0, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/lex.go:329 +0xa2
github.com/openconfig/goyang/pkg/yang.lexQString(0xc8200ca7e0, 0xc8200af1d8)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/lex.go:473 +0x79a
github.com/openconfig/goyang/pkg/yang.(*lexer).NextToken(0xc8200ca7e0, 0xc8200f61c0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/lex.go:178 +0x64d
github.com/openconfig/goyang/pkg/yang.(*parser).next.func1(0xc8200af868)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/parse.go:226 +0x2d
github.com/openconfig/goyang/pkg/yang.(*parser).next(0xc8200af868, 0xc820079c00)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/parse.go:231 +0x86
github.com/openconfig/goyang/pkg/yang.(*parser).nextStatement(0xc8200af868, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/parse.go:297 +0x18e
github.com/openconfig/goyang/pkg/yang.(*parser).nextStatement(0xc8200af868, 0xc820079380)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/parse.go:313 +0x413
github.com/openconfig/goyang/pkg/yang.(*parser).nextStatement(0xc8200af868, 0x589ae0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/parse.go:313 +0x413
github.com/openconfig/goyang/pkg/yang.Parse(0xc8200dc000, 0x2080, 0x7fffd00fd548, 0x13, 0x0, 0x0, 0x0, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/parse.go:184 +0x370
github.com/openconfig/goyang/pkg/yang.(*Modules).Parse(0xc8200ce000, 0xc8200dc000, 0x2080, 0x7fffd00fd548, 0x13, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/modules.go:59 +0x69
github.com/openconfig/goyang/pkg/yang.(*Modules).Read(0xc8200ce000, 0x7fffd00fd548, 0x13, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/modules.go:53 +0xba
main.main()
                /home/camcardo/go/src/github.com/openconfig/goyang/yang.go:185 +0x1769

The problem seems to be "the storage" of errors.

Remark: There could be a hole in the SMIv2 to YANG conversion (rfc6643). YANG only supports four special characters after a backslash (which is correctly handled by goyang), but there is no such impediment in SMIv2 modules; and the conversion does not force to deal with these cases.

[andaru]

Thanks for reporting this. To confirm, the source data you're referring to here is the ASCII-art in description statements within CISCO-IMAGE-TC.mib?

After the deadlock is resolved, would you expect goyang to return an error for this module for this incorrect syntax?

[jccardonar]

Thanks for reporting this. To confirm, the source data you're referring to here is the ASCII-art in description statements within CISCO-IMAGE-TC.mib?

Yes, that should be the issue. I think there was a gap in the SMIV2 to YANG conversion procedure (rfc6643), and these backslashes should be transformed into \\

After the deadlock is resolved, would you expect goyang to return an error for this module for this incorrect syntax?

Goyang can process other MIB files that include a few invalid \ (I think less than 3). I would expect the procedure to be the same. It will be painful to fail due to these errors, but that is life.

[andaru]

Following up on this issue;

First, this is only strictly made invalid by the YANG 1.1 specification. YANG 1.1, after mentioning the four escaped characters (tab, newline, CR and backslash), mentions (s6.1.3, p46):

The backslash MUST NOT be followed by any other character.

YANG 1.0 lacks this statement, leaving the meaning of e.g., \x undefined, but the implication to me is that \x would be a two character literal, though it could also be dropped/ignored given a different interpretation.

So, in this case, for YANG 1.0 modules, we can either ignore or drop (or produce an error, maybe?) the undefined escape characters, in YANG 1.1, the spec says we must produce an error.

Does that sound like a reasonable approach to you? (i.e., allowing YANG 1.0 modules to parse, but failing for YANG 1.1 modules with such undefined escape characters)

(The reason I bring this bug up is I'm doing an issue scrub for YANG 1.1 related issues presently as we plan and do the remaining development work - apologies for the delay)

[jccardonar]

Yes, I think that makes sense.
The issue was raised when processing "MIB Yangs" (rfc6643), which are generated normally using an automatic procedure. I think that rfc was thought with YANG 1.0 in mind. Based on that, I would prefer for the YANG 1.0 procedure to only warn, or fail but add an option for ignoring it.

[andaru]

Sounds good, we will warn or simply ignore those errors for YANG 1.0.

[andaru]

And confirming that in the explicit case of \\\\, that will translate to \\ as expected.