[PATCH] revlog: ensure that flags do not overflow 2 bytes

[Gregory Szorc]

I think having the check in offset_type() to catch all consumers is the
right place. (Another source of the flag is changegroup data via
revlog.addgroup() and cg.deltachunk().)

To clarify, I'm suggesting that instead of truncating "type" like this
patch is doing, we should raise ValueError in offset_type(). We should
never pass in a too large number and IMO this warrants an exception. This
will actively prevent bad data from buggy code being written to a revlog.


On Fri, Nov 25, 2016 at 10:09 AM, Cotizo Sima <cotizo at fb.com> wrote:

> Gregory, I agree, but in this particular case I chose the “deepest” call
> which insert flags into the revlog such that I cover all the possible call
> paths. Although to be fair, another good alternative would be
> revlog._addrevision. I’m happy to move the operation there, if you guys
> find it more appropriate.
>
>
>
> *From: *Gregory Szorc <gregory.szorc at gmail.com>
> *Date: *Friday, November 25, 2016 at 5:27 PM
> *To: *Cotizo Sima <cotizo at fb.com>
> *Cc: *mercurial-devel <mercurial-devel at mercurial-scm.org>
> *Subject: *Re: [PATCH] revlog: ensure that flags do not overflow 2 bytes
>
>
>
> On Fri, Nov 25, 2016 at 5:24 AM, Cotizo Sima <cotizo at fb.com> wrote:
>
> # HG changeset patch
> # User Cotizo Sima <cotizo at fb.com>
> # Date 1480079985 28800
> #      Fri Nov 25 05:19:45 2016 -0800
> # Node ID 4b311b730614941db6409f560ab9451bec74be75
> # Parent  a3163433647108b7bec8fc45896db1c20b18ab21
> revlog: ensure that flags do not overflow 2 bytes
>
> This patch adds a line that ensures we are not setting by mistake a set of
> flags
> overlfowing the 2 bytes they are allocated. Given the way the data is
> packed in
> the revlog header, overflowing 2 bytes will result in setting a wrong
> offset.
>
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -72,6 +72,7 @@
>      return int(q & 0xFFFF)
>
>  def offset_type(offset, type):
> +    type &= 0xFFFF
>      return long(long(offset) << 16 | type)
>
>  _nullhash = hashlib.sha1(nullid)
>
>
>
> I'm a believer in failing fast. If this new code comes into play, we've
> already lost to a bug. I think instead we should raise ValueError if type >
> 65535.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161125/c86bd014/attachment.html>