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 <cot...@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.sz...@gmail.com> > *Date: *Friday, November 25, 2016 at 5:27 PM > *To: *Cotizo Sima <cot...@fb.com> > *Cc: *mercurial-devel <mercurial-devel@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 <cot...@fb.com> wrote: > > # HG changeset patch > # User Cotizo Sima <cot...@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. > > >
_______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
