D865: obsmarker: fix crash when metadata fields are >255 bytes (issue5681)

[ikostia (Kostia Balytskyi)]

ikostia accepted this revision.
ikostia added a comment.


  LGTM

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D865

To: swhitaker, #hg-reviewers, ikostia
Cc: pulkit, ikostia, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D865: obsmarker: fix crash when metadata fields are >255 bytes (issue5681)

[swhitaker (Simon Whitaker)]

swhitaker updated this revision to Diff 2255.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D865?vs=2229=2255

REVISION DETAIL
  https://phab.mercurial-scm.org/D865

AFFECTED FILES
  mercurial/obsolete.py
  tests/test-obsolete-bounds-checking.t

CHANGE DETAILS

diff --git a/tests/test-obsolete-bounds-checking.t 
b/tests/test-obsolete-bounds-checking.t
new file mode 100644
--- /dev/null
+++ b/tests/test-obsolete-bounds-checking.t
@@ -0,0 +1,23 @@
+Create a repo, set the username to something more than 255 bytes, then run hg 
amend on it.
+
+  $ unset HGUSER
+  $ cat >> $HGRCPATH << EOF
+  > [ui]
+  > username = 

 
+  > [extensions]
+  > amend =
+  > [experimental]
+  > stabilization=createmarkers,exchange
+  > EOF
+  $ hg init tmpa
+  $ cd tmpa
+  $ echo a > a
+  $ hg add
+  adding a
+  $ hg commit -m "Initial commit"
+  $ echo a >> a
+  $ hg amend 2>&1 | egrep -v '^(\*\*|  )'
+  transaction abort!
+  rollback completed
+  Traceback (most recent call last):
+  mercurial.error.ProgrammingError: obsstore metadata value cannot be longer 
than 255 bytes (value 
"
 " for key "user" is 285 bytes)
diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -416,6 +416,14 @@
 for key, value in metadata:
 lk = len(key)
 lv = len(value)
+if lk > 255:
+msg = _('obsstore metadata key cannot be longer than 255 bytes'
+' (key "%s" is %u bytes)') % (key, lk)
+raise error.ProgrammingError(msg)
+if lv > 255:
+msg = _('obsstore metadata value cannot be longer than 255 bytes'
+' (value "%s" for key "%s" is %u bytes)') % (value, key, lv)
+raise error.ProgrammingError(msg)
 data.append(lk)
 data.append(lv)
 totalsize += lk + lv



To: swhitaker, #hg-reviewers, ikostia
Cc: pulkit, ikostia, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D865: obsmarker: fix crash when metadata fields are >255 bytes (issue5681)

[ikostia (Kostia Balytskyi)]

ikostia added a comment.


  That is why we raise `ProgrammingError`, not some sort of `UserError`. If 
your extension writes metadata longer than 255 chars, it is a bad extension. 
Your proposal is weird, because it is quietly modifying things, IMO this is 
much worse. I'd rather be forced to change my name to something shorter than 
having it truncated at some arbitrary position.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D865

To: swhitaker, #hg-reviewers, ikostia
Cc: pulkit, ikostia, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D865: obsmarker: fix crash when metadata fields are >255 bytes

[swhitaker (Simon Whitaker)]

swhitaker added a comment.


  @ikostia Sure, I can change this.
  
  What should we do if the metadata value is not under user control? In the 
case of a username longer than 255 bytes, the user can change that in their 
hgrc (although it's debatable whether they should; "error, your name is too 
long" is a pretty poor UX, especially as this limit isn't enforced across 
Mercurial). But it's possible that functionality exists (or could be added in 
the future) that writes values to the metadata dictionary that the user can't 
easily fix. (e.g. a contrived example: we decide it would be a great idea to 
add the old commit message as metadata when performing a metaedit. If the 
commit message is > 255 bytes the user will have no simple way of fixing that, 
since fixing it would require the functionality that is now broken.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D865

To: swhitaker, #hg-reviewers, ikostia
Cc: pulkit, ikostia, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D865: obsmarker: fix crash when metadata fields are >255 bytes

[pulkit (Pulkit Goyal)]

pulkit added a comment.


  Also we append the issue number in the commit message at the end like 
`(issue)` so that the Bugzilla bot can automatically close that. For 
reference: 
https://www.mercurial-scm.org/repo/hg-committed/log?rev=issue=20

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D865

To: swhitaker, #hg-reviewers, ikostia
Cc: pulkit, ikostia, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D865: obsmarker: fix crash when metadata fields are >255 bytes

[ikostia (Kostia Balytskyi)]

ikostia requested changes to this revision.
ikostia added a comment.
This revision now requires changes to proceed.


  I would rather see this thing do `raise error.ProrgrammingError('metadata 
cannot be longer than 255 bytes')`  (or some other message).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D865

To: swhitaker, #hg-reviewers, ikostia
Cc: ikostia, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D865: obsmarker: fix crash when metadata fields are >255 bytes

[swhitaker (Simon Whitaker)]

swhitaker created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Various mutators fail when attempting to write obsmarkers with
  metadata fields longer than 255 bytes, since the length of
  mwetadata fields is stored in u8s. This change truncates long
  strings to the first 255 bytes so that they can be written to
  the V1 obsmarker format without crashing hg.
  
  Fixes https://bz.mercurial-scm.org/show_bug.cgi?id=5681

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D865

AFFECTED FILES
  mercurial/obsolete.py
  tests/test-obsolete-bounds-checking.t

CHANGE DETAILS

diff --git a/tests/test-obsolete-bounds-checking.t 
b/tests/test-obsolete-bounds-checking.t
new file mode 100644
--- /dev/null
+++ b/tests/test-obsolete-bounds-checking.t
@@ -0,0 +1,17 @@
+Create a repo, set the username to something more than 255 bytes, then run hg 
amend on it.
+
+  $ cat >> $HGRCPATH << EOF
+  > [ui]
+  > username = 

 
+  > [extensions]
+  > amend =
+  > EOF
+  $ hg init tmpa
+  $ cd tmpa
+  $ echo a > a
+  $ hg add
+  adding a
+  $ hg commit -m "Initial commit"
+  $ echo a >> a
+  $ hg amend
+  saved backup bundle to 
$TESTTMP/tmpa/.hg/strip-backup/78a4d7b136df-b05e3630-amend.hg (glob)
diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -414,16 +414,16 @@
 data.extend(parents)
 totalsize = _calcsize(format)
 for key, value in metadata:
-lk = len(key)
-lv = len(value)
+lk = min(len(key), 255)
+lv = min(len(value), 255)
 data.append(lk)
 data.append(lv)
 totalsize += lk + lv
 data[0] = totalsize
 data = [_pack(format, *data)]
 for key, value in metadata:
-data.append(key)
-data.append(value)
+data.append(key[:255])
+data.append(value[:255])
 return ''.join(data)
 
 def _fm1readmarkers(data, off, stop):



To: swhitaker, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel