This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2948

wireproto: syntax for encoding CBOR into frames
ClosedPublic

Authored by indygreg on Mar 26 2018, 2:12 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGcc5a040fe150: wireproto: syntax for encoding CBOR into frames
Summary

We just vendored a library for encoding and decoding the CBOR
data format. While the intent of that vendor was to support state
files, CBOR is really a nice data format. It is extensible and
compact.

I've been feeling dirty inventing my own data formats for
frame payloads. While custom formats can always beat out a generic
format, there is a cost to be paid in terms of implementation,
comprehension, etc. CBOR is compact enough that I'm not too
worried about efficiency loss. I think the benefits of using
a standardized format outweigh rolling our own formats. So
I plan to make heavy use of CBOR in the wire protocol going
forward.

This commit introduces support for encoding CBOR data in frame
payloads to our function to make a frame from a human string.
We do need to employ some low-level Python code in order to
evaluate a string as a Python expression. But other than that,
this should hopefully be pretty straightforward.

Unit tests for this function have been added.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Mar 26 2018, 2:12 PM
indygreg updated this revision to Diff 7327.Mar 26 2018, 5:35 PM
indygreg updated this revision to Diff 7352.Mar 28 2018, 6:05 PM
indygreg updated this revision to Diff 7437.Mar 30 2018, 4:32 PM
durin42 added a subscriber: durin42.Apr 3 2018, 11:35 AM

I have misgivings about this code that serves testing living in core, but I don't see a better solution offhand.

durin42 accepted this revision.Apr 3 2018, 11:35 AM
This revision is now accepted and ready to land.Apr 3 2018, 11:35 AM
Closed by commit rHGcc5a040fe150: wireproto: syntax for encoding CBOR into frames (authored by indygreg). · Explain WhyApr 3 2018, 12:49 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Apr 4 2018, 9:21 AM
yuja added inline comments.
mercurial/utils/stringutil.py
365

Can't we use ast.literal_eval() instead of unsafe eval() ?

https://docs.python.org/2.7/library/ast.html#ast.literal_eval

Revision Contents
Changeset List

Diff 7437

mercurial/debugcommands.py

of the type. The strings are defined in ``wireprotoframing.py``. e.g. of the type. The strings are defined in ``wireprotoframing.py``. e.g.
``command-name``. ``command-name``.
``stream-flags`` and ``flags`` are a ``|`` delimited list of flag ``stream-flags`` and ``flags`` are a ``|`` delimited list of flag
components. Each component (and there can be just one) can be an integer components. Each component (and there can be just one) can be an integer
or a flag name for stream flags or frame flags, respectively. Values are or a flag name for stream flags or frame flags, respectively. Values are
resolved to integers and then bitwise OR'd together. resolved to integers and then bitwise OR'd together.
``payload`` is is evaluated as a Python byte string literal. ``payload`` represents the raw frame payload. If it begins with
``cbor:``, the following string is evaluated as Python code and the
resulting object is fed into a CBOR encoder. Otherwise it is interpreted
as a Python byte string literal.
""" """
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
if opts['localssh'] and not repo: if opts['localssh'] and not repo:
raise error.Abort(_('--localssh requires a repository')) raise error.Abort(_('--localssh requires a repository'))
if opts['peer'] and opts['peer'] not in ('raw', 'ssh1', 'ssh2'): if opts['peer'] and opts['peer'] not in ('raw', 'ssh1', 'ssh2'):
raise error.Abort(_('invalid value for --peer'), raise error.Abort(_('invalid value for --peer'),

mercurial/utils/stringutil.py

# stringutil.py - utility for generic string formatting, parsing, etc. # stringutil.py - utility for generic string formatting, parsing, etc.
# #
# Copyright 2005 K. Thananchayan <thananck@yahoo.com> # Copyright 2005 K. Thananchayan <thananck@yahoo.com>
# Copyright 2005-2007 Matt Mackall <mpm@selenic.com> # Copyright 2005-2007 Matt Mackall <mpm@selenic.com>
# Copyright 2006 Vadim Gelfer <vadim.gelfer@gmail.com> # Copyright 2006 Vadim Gelfer <vadim.gelfer@gmail.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import __future__
import codecs import codecs
import re as remod import re as remod
import textwrap import textwrap
from ..i18n import _ from ..i18n import _
from .. import ( from .. import (
encoding, encoding,
'never': False} 'never': False}
def parsebool(s): def parsebool(s):
"""Parse s into a boolean. """Parse s into a boolean.
If s is not a valid boolean, returns None. If s is not a valid boolean, returns None.
""" """
return _booleans.get(s.lower(), None) return _booleans.get(s.lower(), None)
def evalpython(s):
"""Evaluate a string containing a Python expression.
THIS FUNCTION IS NOT SAFE TO USE ON UNTRUSTED INPUT. IT'S USE SHOULD BE
LIMITED TO DEVELOPER-FACING FUNCTIONALITY.
"""
globs = {
r'__builtins__': {
r'None': None,
r'False': False,
r'True': True,
r'int': int,
r'set': set,
r'tuple': tuple,
# Don't need to expose dict and list because we can use
# literals.
},
}
# We can't use eval() directly because it inherits compiler
# flags from this module and we need unicode literals for Python 3
# compatibility.
code = compile(s, r'<string>', r'eval',
__future__.unicode_literals.compiler_flag, True)
return eval(code, globs, {})
yujaUnsubmitted
Not Done

Can't we use ast.literal_eval() instead of unsafe eval() ?

https://docs.python.org/2.7/library/ast.html#ast.literal_eval

yuja: Can't we use `ast.literal_eval()` instead of unsafe `eval()` ? https://docs.python.org/2.

mercurial/wireprotoframing.py

from __future__ import absolute_import from __future__ import absolute_import
import struct import struct
from .i18n import _ from .i18n import _
from .thirdparty import ( from .thirdparty import (
attr, attr,
cbor,
) )
from . import ( from . import (
error, error,
util, util,
) )
from .utils import ( from .utils import (
stringutil, stringutil,
) )
frame[7] = (typeid << 4) | flags frame[7] = (typeid << 4) | flags
frame[8:] = payload frame[8:] = payload
return frame return frame
def makeframefromhumanstring(s): def makeframefromhumanstring(s):
"""Create a frame from a human readable string """Create a frame from a human readable string
DANGER: NOT SAFE TO USE WITH UNTRUSTED INPUT BECAUSE OF POTENTIAL
eval() USAGE. DO NOT USE IN CORE.
Strings have the form: Strings have the form:
<request-id> <stream-id> <stream-flags> <type> <flags> <payload> <request-id> <stream-id> <stream-flags> <type> <flags> <payload>
This can be used by user-facing applications and tests for creating This can be used by user-facing applications and tests for creating
frames easily without having to type out a bunch of constants. frames easily without having to type out a bunch of constants.
Request ID and stream IDs are integers. Request ID and stream IDs are integers.
Stream flags, frame type, and flags can be specified by integer or Stream flags, frame type, and flags can be specified by integer or
named constant. named constant.
Flags can be delimited by `|` to bitwise OR them together. Flags can be delimited by `|` to bitwise OR them together.
If the payload begins with ``cbor:``, the following string will be
evaluated as Python code and the resulting object will be fed into
a CBOR encoder. Otherwise, the payload is interpreted as a Python
byte string literal.
""" """
fields = s.split(b' ', 5) fields = s.split(b' ', 5)
requestid, streamid, streamflags, frametype, frameflags, payload = fields requestid, streamid, streamflags, frametype, frameflags, payload = fields
requestid = int(requestid) requestid = int(requestid)
streamid = int(streamid) streamid = int(streamid)
finalstreamflags = 0 finalstreamflags = 0
finalflags = 0 finalflags = 0
validflags = FRAME_TYPE_FLAGS[frametype] validflags = FRAME_TYPE_FLAGS[frametype]
for flag in frameflags.split(b'|'): for flag in frameflags.split(b'|'):
if flag in validflags: if flag in validflags:
finalflags |= validflags[flag] finalflags |= validflags[flag]
else: else:
finalflags |= int(flag) finalflags |= int(flag)
if payload.startswith(b'cbor:'):
payload = cbor.dumps(stringutil.evalpython(payload[5:]), canonical=True)
else:
payload = stringutil.unescapestr(payload) payload = stringutil.unescapestr(payload)
return makeframe(requestid=requestid, streamid=streamid, return makeframe(requestid=requestid, streamid=streamid,
streamflags=finalstreamflags, typeid=frametype, streamflags=finalstreamflags, typeid=frametype,
flags=finalflags, payload=payload) flags=finalflags, payload=payload)
def parseheader(data): def parseheader(data):
"""Parse a unified framing protocol frame header from a buffer. """Parse a unified framing protocol frame header from a buffer.

tests/test-wireproto-serverreactor.py

payload)) payload))
def sendcommandframes(reactor, stream, rid, cmd, args, datafh=None): def sendcommandframes(reactor, stream, rid, cmd, args, datafh=None):
"""Generate frames to run a command and send them to a reactor.""" """Generate frames to run a command and send them to a reactor."""
return sendframes(reactor, return sendframes(reactor,
framing.createcommandframes(stream, rid, cmd, args, framing.createcommandframes(stream, rid, cmd, args,
datafh)) datafh))
class FrameHumanStringTests(unittest.TestCase):
def testbasic(self):
self.assertEqual(ffs(b'1 1 0 1 0 '),
b'\x00\x00\x00\x01\x00\x01\x00\x10')
self.assertEqual(ffs(b'2 4 0 1 0 '),
b'\x00\x00\x00\x02\x00\x04\x00\x10')
self.assertEqual(ffs(b'2 4 0 1 0 foo'),
b'\x03\x00\x00\x02\x00\x04\x00\x10foo')
def testcborint(self):
self.assertEqual(ffs(b'1 1 0 1 0 cbor:15'),
b'\x01\x00\x00\x01\x00\x01\x00\x10\x0f')
self.assertEqual(ffs(b'1 1 0 1 0 cbor:42'),
b'\x02\x00\x00\x01\x00\x01\x00\x10\x18*')
self.assertEqual(ffs(b'1 1 0 1 0 cbor:1048576'),
b'\x05\x00\x00\x01\x00\x01\x00\x10\x1a'
b'\x00\x10\x00\x00')
self.assertEqual(ffs(b'1 1 0 1 0 cbor:0'),
b'\x01\x00\x00\x01\x00\x01\x00\x10\x00')
self.assertEqual(ffs(b'1 1 0 1 0 cbor:-1'),
b'\x01\x00\x00\x01\x00\x01\x00\x10 ')
self.assertEqual(ffs(b'1 1 0 1 0 cbor:-342542'),
b'\x05\x00\x00\x01\x00\x01\x00\x10:\x00\x05:\r')
def testcborstrings(self):
# String literals should be unicode.
self.assertEqual(ffs(b"1 1 0 1 0 cbor:'foo'"),
b'\x04\x00\x00\x01\x00\x01\x00\x10cfoo')
self.assertEqual(ffs(b"1 1 0 1 0 cbor:b'foo'"),
b'\x04\x00\x00\x01\x00\x01\x00\x10Cfoo')
self.assertEqual(ffs(b"1 1 0 1 0 cbor:u'foo'"),
b'\x04\x00\x00\x01\x00\x01\x00\x10cfoo')
def testcborlists(self):
self.assertEqual(ffs(b"1 1 0 1 0 cbor:[None, True, False, 42, b'foo']"),
b'\n\x00\x00\x01\x00\x01\x00\x10\x85\xf6\xf5\xf4'
b'\x18*Cfoo')
def testcbordicts(self):
self.assertEqual(ffs(b"1 1 0 1 0 "
b"cbor:{b'foo': b'val1', b'bar': b'val2'}"),
b'\x13\x00\x00\x01\x00\x01\x00\x10\xa2'
b'CbarDval2CfooDval1')
class FrameTests(unittest.TestCase): class FrameTests(unittest.TestCase):
def testdataexactframesize(self): def testdataexactframesize(self):
data = util.bytesio(b'x' * framing.DEFAULT_MAX_FRAME_SIZE) data = util.bytesio(b'x' * framing.DEFAULT_MAX_FRAME_SIZE)
stream = framing.stream(1) stream = framing.stream(1)
frames = list(framing.createcommandframes(stream, 1, b'command', frames = list(framing.createcommandframes(stream, 1, b'command',
{}, data)) {}, data))
self.assertEqual(frames, [ self.assertEqual(frames, [