Re: [tor-bugs] #28940 [Obfuscation/Pluggable transport]: Add support for LOG to goptlib

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#28940: Add support for LOG to goptlib
---------------------------------------------+-----------------------------
 Reporter:  dcf                              |          Owner:  dcf
     Type:  enhancement                      |         Status:
                                             |  needs_review
 Priority:  Medium                           |      Milestone:
Component:  Obfuscation/Pluggable transport  |        Version:
 Severity:  Normal                           |     Resolution:
 Keywords:  goptlib                          |  Actual Points:
Parent ID:                                   |         Points:  0.3
 Reviewer:                                   |        Sponsor:  Sponsor19
---------------------------------------------+-----------------------------
Changes (by dcf):

 * status:  merge_ready => needs_review


Comment:

 attachment:Bug-28940-add-support-for-LOG.2.patch has some minor changes.
 The first is that I changed "log level" to "log severity" to match the
 terminology of the spec.

 The second change is to better conform with the letter of the spec
 regarding the value of `SEVERITY=`. But the way the spec is worded may be
 an oversight, so I want to check. Formerly I defined the log severity
 constants as plain strings. This was safe because the function escaped the
 severity string, so no matter what garbage the caller provided, it would
 not violate the global "any US-ASCII character but NUL or NL".

 But while the spec specifies quoting for `MESSAGE=`, it
 [https://gitweb.torproject.org/torspec.git/tree/pt-
 spec.txt?id=d890052d5525a09829c798ab0ad6dcdcede1a8ef#n599 does not
 actually say] that `SEVERITY=` may be quoted. That is, while this
 certainly conforms:
 {{{
 LOG SEVERITY=debug MESSAGE="hello world"
 }}}
 this may not conform:
 {{{
 LOG SEVERITY="debug" MESSAGE="hello world"
 }}}
 But if we don't quote the severity string, then we need to prevent callers
 from providing garbage values for it. I took some inspiration from
 [https://github.com/ahf/goptlib/commit/6815c6e0bcb5547e9d4328b8d7a3a10746094b1b
 #diff-308aaa719425f4333033c97cfb3b9868R10 ahf's patch]. I made the log
 severity constants be instances of an unexported struct type; i.e., it's
 not possible to create additional instances of the style from outside the
 package. In short it looks like this:
 {{{
 // Unexported type to prevent external callers from inventing new
 severities.
 type logSeverity struct {
         string
 }

 // Severity levels for the Log function.
 var (
         LogSeverityError   = logSeverity{"error"}
         LogSeverityWarning = logSeverity{"warning"}
         LogSeverityNotice  = logSeverity{"notice"}
         LogSeverityInfo    = logSeverity{"info"}
         LogSeverityDebug   = logSeverity{"debug"}
 )
 }}}
 And the body of `Log` changed from
 {{{
 line("LOG", "SEVERITY="+encodeCString(severity),
 "MESSAGE="+encodeCString(message))
 }}}
 to
 {{{
 line("LOG", "SEVERITY="+severity.string,
 "MESSAGE="+encodeCString(message))
 }}}
 So the question is, is this strict enumeration necessary, or is the spec
 meant to allow quoting of `SEVERITY=`? It's allowed by tor; but should we
 specify it? As it stands, an implementation would be totally justified in
 parsing a LOG line with a regex, something like: `^LOG
 SEVERITY=(error|warning|notice|info|debug)
 MESSAGE=(\w+|"(\\[0-9]{1,3}|\\n|\\t|\\r|\\"|\\\\|[^\\])*")$`.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28940#comment:12>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs