ConfigObj is able to write bad branch.conf which is not possible to read back

please attach branch.conf from .bzr/branch/branch.conf in your affected tree. If that file contains sensitive information please remove it. I'm interested in [commit_data] section.

I'm curious why this was switched to qbzr. I'm not using qbzr -- I'm using the command line -- and I did not use any of the q* commands. Commands involved were:

bzr commit -F <wrong file>
bzr uncommit

Pat Tressel wrote:
> I'm curious why this was switched to qbzr. I'm not using qbzr -- I'm
> using the command line -- and I did not use any of the q* commands.
> Commands involved were:

Because the traceback clearly shows that the error is raised from within
a hook function registered by qbzr. This doesn't guarantee that qbzr is
the cause, but it's currently the primary suspect.

You could try those commands again with --no-plugins; that may
work around the problem for you.

Pat Tressel пишет:
> I'm curious why this was switched to qbzr. I'm not using qbzr -- I'm
> using the command line -- and I did not use any of the q* commands.
> Commands involved were:
>
> bzr commit -F <wrong file>
> bzr uncommit

To recover your affected tree you just need delete [commit_data] section
and all its contents from branch.conf.

--
All the dude wanted was his rug back

OK, this problem affects not only uncommit. After the weird commit message going to branch.conf there is no way to work with the branch at all. I've reproduced it with simply branching the same lp project and replacing default branch.conf with supplied by bug reporter. I have the same traceback but now you don't know that qbzr is the suspect:

C:\Temp\bug-710410\sahana-eden>bzr info
bzr: ERROR: bzrlib.util.configobj.configobj.ConfigObjError: Parsing failed with several errors.
First error at line 155.

Traceback (most recent call last):
  File "bzrlib\commands.pyo", line 923, in exception_to_return_code
  File "bzrlib\commands.pyo", line 1123, in run_bzr
  File "bzrlib\commands.pyo", line 691, in run_argv_aliases
  File "bzrlib\commands.pyo", line 710, in run
  File "bzrlib\cleanup.pyo", line 135, in run_simple
  File "bzrlib\cleanup.pyo", line 165, in _do_with_cleanups
  File "bzrlib\commands.pyo", line 1138, in ignore_pipe
  File "bzrlib\builtins.pyo", line 1495, in run
  File "bzrlib\info.pyo", line 324, in show_bzrdir_info
  File "bzrlib\bzrdir.pyo", line 1447, in open_workingtree
  File "bzrlib\workingtree.pyo", line 3119, in open
  File "bzrlib\workingtree_4.pyo", line 1499, in _open
  File "bzrlib\bzrdir.pyo", line 1430, in open_branch
  File "bzrlib\branch.pyo", line 2098, in open
  File "bzrlib\branch.pyo", line 2830, in __init__
  File "bzrlib\branch.pyo", line 2440, in __init__
  File "bzrlib\branch.pyo", line 98, in __init__
  File "bzrlib\branch.pyo", line 2814, in _open_hook
  File "bzrlib\branch.pyo", line 3041, in get_stacked_on_url
  File "bzrlib\branch.pyo", line 1157, in _get_config_location
  File "bzrlib\config.pyo", line 197, in get_user_option
  File "bzrlib\config.pyo", line 1021, in _get_user_option
  File "bzrlib\config.pyo", line 529, in _get_user_option
  File "bzrlib\config.pyo", line 1253, in _get_parser
  File "bzrlib\config.pyo", line 1786, in _get_configobj
  File "bzrlib\config.pyo", line 149, in ConfigObj
  File "bzrlib\util\configobj\configobj.pyo", line 1223, in __init__
  File "bzrlib\util\configobj\configobj.pyo", line 1306, in _load
ConfigObjError: Parsing failed with several errors.
First error at line 155.

bzr 2.3b5 on python 2.6.6 (Windows-XP-5.1.2600-SP3)
arguments: ['C:\\Program Files\\Bazaar\\bzr.EXE', 'info']
encoding: 'cp1251', fsenc: 'mbcs', lang: None
plugins:
  acad C:\work\Bazaar\plugins\acad [0.8.0]
  bzrtools C:\Program Files\Bazaar\plugins\bzrtools [2.3.0]
  colo C:\Program Files\Bazaar\plugins\colo [0.2.0]
  explorer C:\Program Files\Bazaar\plugins\explorer [1.1.3dev]
  format1 C:\work\Bazaar\plugins\format1 [unknown]
  launchpad C:\Program Files\Bazaar\plugins\launchpad [2.3b5]
  qbzr C:\Program Files\Bazaar\plugins\qbzr [0.20.0dev1]
  rewrite C:\Program Files\Bazaar\plugins\rewrite [0.6.2dev]
  scmproj C:\work\Bazaar\plugins\scmproj [0.6.1]
  x_bit C:\work\Bazaar\plugins\x_bit [1.0.0]

*** Bazaar has encountered an internal error. This probably indicates a
    bug in Bazaar. You can help us fix it by filing a bug report at
        https://bugs.laun...

Should I send the bug report to ConfigObj authors?

Alexander Belchenko пишет:
> Should I send the bug report to ConfigObj authors?

Sent.

--
All the dude wanted was his rug back

OK, I see now. ConfigObj puts the multiline string as

foo = """spam
eggs
fish
salade
"""

quoted ith triple ".

Unfortunately the date we saved as part of uncommit has multiple """ inside. And that might broke the parsing.

So ConfigObj probably should escape the input, but if it does not then QBzr should escape (or encode) the data by self. That will be format change though.

If you're going to break the format, why not write the commit message to a different file in its raw form. That way you don't have to worry about possible quoting problems or inadvertently breaking the branch.

> So, the problem in the underlying ConfigObj: it can write bad conf file and
> unable to read it back.
> I'm not quite understand how to fix it in the QBzr.
>
>

> ** Summary changed:
>
> + ConfigObj is able to write bad branch.conf which is not possible to read
> back
>

bzr could get any sort of file thrown at it via -F, including ones that have
text that looks like meaningful strings in branch.conf, or binary data,
or...is there an equivalent of cross-site-scripting when reading branch.conf
(not likely, if it doesn't direct bzr to take actions, but just as an
example...).

Some options to protect against that:

1) Check whether it appears to be a reasonable text message, e.g.: Do a
trial run at running it through the parser *as it's being read in* and see
if it will cause trouble. Do whatever check is appropriate on the system
for binary vs. text. Scan it for bzr keywords. Ask the user if they really
want that message.

2) Quarantine messages, or at least ones that seem iffy: Escape the text or
run a simple encoding-to-text on it. Store it as a separate file (at a
known relative path w/i .bzr/branch) and only put the filename in
branch.conf. This would apply not just to the message stored in
branch.conf, but wherever else it is stored in the collection of revisions.

To recover your affected tree you just need delete [commit_data] section and
> all its contents from branch.conf.
>

;-) Getting it back from launchpad worked. But that brings up another
point (alluded to above), which is... Isn't that message stored in some
other place as well, in repository directory itself? It doesn't appear to
have caused problems for commands that access the repository, but if stored
there, it could resurface via uncommits.

-- Pat

Aha -- I see people have already made the suggestions below. (I'm using
gmail and it started a fresh thread due to the new subject. ;-) The only
ones that remain are a) perhaps ask the user if they intended that message,
and b) if it's also in the repository, might need to deal w/ it there, at
least to prevent it from being brought back into branch.conf as
un-escaped/un-encoded/not-stored-as-file text by an uncommit.

-- Pat

bzr could get any sort of file thrown at it via -F, including ones that have
> text that looks like meaningful strings in branch.conf, or binary data,
> or...is there an equivalent of cross-site-scripting when reading branch.conf
> (not likely, if it doesn't direct bzr to take actions, but just as an
> example...).
>
> Some options to protect against that:
>
> 1) Check whether it appears to be a reasonable text message, e.g.: Do a
> trial run at running it through the parser *as it's being read in* and see
> if it will cause trouble. Do whatever check is appropriate on the system
> for binary vs. text. Scan it for bzr keywords. Ask the user if they really
> want that message.
>
> 2) Quarantine messages, or at least ones that seem iffy: Escape the text
> or run a simple encoding-to-text on it. Store it as a separate file (at a
> known relative path w/i .bzr/branch) and only put the filename in
> branch.conf. This would apply not just to the message stored in
> branch.conf, but wherever else it is stored in the collection of revisions.
>
> To recover your affected tree you just need delete [commit_data] section
>> and all its contents from branch.conf.
>>
>
> ;-) Getting it back from launchpad worked. But that brings up another
> point (alluded to above), which is... Isn't that message stored in some
> other place as well, in repository directory itself? It doesn't appear to
> have caused problems for commands that access the repository, but if stored
> there, it could resurface via uncommits.
>

On Mon, January 31, 2011 10:48 am, Pat Tressel wrote:
> Aha -- I see people have already made the suggestions below. (I'm using
> gmail and it started a fresh thread due to the new subject. ;-) The only
> ones that remain are a) perhaps ask the user if they intended that
> message,
> and b) if it's also in the repository, might need to deal w/ it there, at
> least to prevent it from being brought back into branch.conf as
> un-escaped/un-encoded/not-stored-as-file text by an uncommit.

As I understand it, the copy in branch.conf is only kept there temporarily
to remember what the last commit message was, so that the commit command
can offer the same message when re-committing an uncommitted revision, for
example. It has no relation to how the commit message is actually stored
in the branch revision data.

Gordon Tyler пишет:
> As I understand it, the copy in branch.conf is only kept there temporarily
> to remember what the last commit message was, so that the commit command
> can offer the same message when re-committing an uncommitted revision, for
> example. It has no relation to how the commit message is actually stored
> in the branch revision data.

Exactly.

--
All the dude wanted was his rug back

On Mon, Jan 31, 2011 at 03:12:36PM -0000, Gordon Tyler wrote:
> If you're going to break the format, why not write the commit message to
> a different file in its raw form. That way you don't have to worry about
> possible quoting problems or inadvertently breaking the branch.
It might also make sense to put the support for remembering old commit messages
into bzr core rather than qbzr and bzr-gtk separately (they each have their own
implementatin at the moment).

Cheers,

Jelmer

    > It might also make sense to put the support for remembering old commit messages
    > into bzr core rather than qbzr and bzr-gtk separately (they each have their own
    > implementatin at the moment).

+1

This sounds like a bug in the configobj quoting algorithm but while we
wait for an upstream fix, I think we should find a workaround.

> ... if it's also in the repository, might need to deal w/ it there, at
>> least to prevent it from being brought back into branch.conf as
>> un-escaped/un-encoded/not-stored-as-file text by an uncommit.
>>
>
> As I understand it, the copy in branch.conf is only kept there temporarily
> to remember what the last commit message was, so that the commit command can
> offer the same message when re-committing an uncommitted revision, for
> example. It has no relation to how the commit message is actually stored in
> the branch revision data.
>

So, an uncommit does not bring back the message from the (now) head revision
into branch.conf? The point was, if a message can get brought back into
branch.conf from the repository, but it's stored as verbatim original text
in the repository, you may need to add the same protection on that means of
inserting text into branch.conf as you would against the user inserting that
text with bzr commit -F, or, alternatively, support storing messages in the
repository in escaped/encoded/file-pointer format.

-- Pat

On 1/31/2011 4:59 PM, Pat Tressel wrote:
>> As I understand it, the copy in branch.conf is only kept there temporarily
>> to remember what the last commit message was, so that the commit command can
>> offer the same message when re-committing an uncommitted revision, for
>> example. It has no relation to how the commit message is actually stored in
>> the branch revision data.
>
> So, an uncommit does not bring back the message from the (now) head revision
> into branch.conf? The point was, if a message can get brought back into
> branch.conf from the repository, but it's stored as verbatim original text
> in the repository, you may need to add the same protection on that means of
> inserting text into branch.conf as you would against the user inserting that
> text with bzr commit -F, or, alternatively, support storing messages in the
> repository in escaped/encoded/file-pointer format.

The uncommit comand does bring the message back from the revision data
and store it in branch.conf for the commit to "remember" it.

Anyway, my point was that the storing of the commit message in
branch.conf is merely temporary and is probably not subject to the same
level of testing that storage of commit messages in revision data is.
Suffice to say that there is no problem storing this kind of commit
message in the revision data.

Ok, so as I suspected the problem is that the text being embedded in a value has triple quotes in it. As it only has triple double quotes (""") so ConfigObj *should* be using triple single quotes - so this is the particular bug that is biting you.

However for storing arbitrary data it is not safe to use multiline values that contain both triple single quotes and triple double quotes as ConfigObj then has no way to include the multiline value and will raise an exception when writing.

This is all *supposed* to be done in "_get_triple_quote" on line 1794 of your embedded version of configobj.py.

However - there is a horrible bug. tsquot and tdquot have [what look like give their names] the wrong values and configobj will *always* use the wrong quotes when quoting values that have triple quotes in them! Aargh...

So, swapping over lines 1798 and 1800 of *your* version of configobj.py will fix the immediate problem - but potentially still leave you with a difficulty where values have *both* triple double quotes and triple single quotes.

I'll fix the bug upstream of course.

@Martin Pool: is it OK for us to make the change suggested by Michael Foord? If yes, then I'd like to have it at least in 2.3, if possible in 2.2 as well. I can prepare the patch, if you're agree.

@bialix, sure, that makes sense to me. By all means do it in 2.2.

Thanks, mfoord.

One possible approach would be to store the repr of the string. This will escape quotes and so avoids the problem. You then use the "string_escape" codec to unescape again.

This would also escape newlines, so would turn strings into a single line. If you still want them to be human readable then you could manually replace "\n" with a newline in the escaped string and reverse this before unescaping.

e.g. to write:

    value = repr(the_real_value).replace('\\n', '\n')

and to read:

    the_real_value = value.replace('\n', '\\n').decode('string_escape')

Michael Foord пишет:
> One possible approach would be to store the repr of the string. This
> will escape quotes and so avoids the problem. You then use the
> "string_escape" codec to unescape again.
>
> This would also escape newlines, so would turn strings into a single
> line. If you still want them to be human readable then you could
> manually replace "\n" with a newline in the escaped string and reverse
> this before unescaping.
>
> e.g. to write:
>
> value = repr(the_real_value).replace('\\n', '\n')
>
> and to read:
>
> the_real_value = value.replace('\n', '\\n').decode('string_escape')
>

Thank you, Michael.

But actually for our purpose to store arbitrary non-ascii text we need
'unicode-escape' codec, I think. We can use this approach in QBzr
library that has the code to trigger this bug. Although for QBzr it
means we have to break backward compatibility with older QBzr versions.

--
All the dude wanted was his rug back

Well, you could change the value name and load the new value if it exists (using the new code) or the old value if it is there (for backwards compatibility). The new version could "auto-migrate" older versions.

Michael Foord пишет:
> Well, you could change the value name and load the new value if it
> exists (using the new code) or the old value if it is there (for
> backwards compatibility). The new version could "auto-migrate" older
> versions.

Yes, of course. That's why I said about breaking backward compatibility:
older versions of QBzr won't be able to read new option. But that's OK.

@Gordon, for some reason I haven't seen your message.

"If you're going to break the format, why not write the commit message to a different file in its raw form. That way you don't have to worry about possible quoting problems or inadvertently breaking the branch."

Because bzrlib.config provides me a very nice API to deal with branch.conf. And strictly to say I should not write any arbitrary file inside .bzr control directory, because it may break future additions to bzr itself?

@Gordon: Yes, using branch.conf wasn't a good idea in retrospect. But it was the less evil alternative in these days and bzrlib.config still doesn't support tree.conf and multiplying the config files may not be the best route either.

And this commit/uncommit data should be supported by bzr core *anyway*. We'll get there and we should probably file a bug for it if there isn't one already.

This bug was fixed in the package configobj - 4.7.2+ds-2

---------------
configobj (4.7.2+ds-2) unstable; urgency=low

  [ Stefano Rivera ]
  * Don't leak uid and umask into source tarball and set -e.
  * Bumped Standards-Version to 3.9.1 (no changes needed).
  * Enable test suites.
    - Build Depend on python-all, python-unittest2.
    - New patch: report-doctest-failure.diff: Fail on failures.
    - New patch: py27-test.diff: Convert float-comparing doctests to unit
      tests.
  * Wrap long lines in debian/control.
  * Merge Build-Depends-Indep into Build-Depends (no arch-dependant packages).
  * Switch to dh_python2
    - Use X-Python-Version (requires python-all >= 2.6.5-13~).
    - Use ${python:Breaks}.
  * Update my e-mail address.
  * Use DEP5 format in debian/copyright.

  [ Jelmer Vernooij ]
  * Properly handle triple quotes. Closes: #618349, LP: #710410
  * Add myself to uploaders.
 -- Jelmer Vernooij <email address hidden> Sun, 20 Mar 2011 13:20:29 +0000

I found another reason why this happens, On windows 7 the file

C:\Users\myuser1\AppData\Roaming\bazaar\2.0\qbzr.conf got somehow corrupted

(sympom bzr command line works fine, but from tortoisebzr gives the same error).

Fix was to rename the file, relaunch Bazaar explorer and let them create a new one.

The complete bzr error I was getting was:

bzr: ERROR: bzrlib.util.configobj.configobj.ConfigObjError: Parsing failed with several errors.
First error at line 1.

Traceback (most recent call last):
  File "C:/Program Files (x86)/Bazaar/plugins\qbzr\lib\commands.py", line 176, in run
  File "C:/Program Files (x86)/Bazaar/plugins\qbzr\lib\commands.py", line 510, in _qbzr_run
  File "bzrlib\lazy_import.pyo", line 129, in __call__
  File "C:/Program Files (x86)/Bazaar/plugins\qbzr\lib\log.py", line 113, in __init__
  File "C:/Program Files (x86)/Bazaar/plugins\qbzr\lib\util.py", line 339, in restoreSize
  File "C:/Program Files (x86)/Bazaar/plugins\qbzr\lib\util.py", line 128, in get_option
  File "C:/Program Files (x86)/Bazaar/plugins\qbzr\lib\util.py", line 109, in _load
  File "bzrlib\util\configobj\configobj.pyo", line 1223, in __init__
  File "bzrlib\util\configobj\configobj.pyo", line 1306, in _load
ConfigObjError: Parsing failed with several errors.
First error at line 1.

bzr 2.4.2 on python 2.6.6 (win32)
arguments: ['C:\\Program Files (x86)\\Bazaar\\tbzrcommand.exe', '--command=log', '--file=C:\\src\\MainSource\\features\\DebuggerNewSyncLogic4']
encoding: 'cp1252', fsenc: 'mbcs', lang: 'en'
plugins:
  bzrtools C:\Program Files (x86)\Bazaar\plugins\bzrtools [2.4.1]
  changelog_merge C:\Program Files (x86)\Bazaar\plugins\changelog_merge [2.4.2]
  colo C:\Program Files (x86)\Bazaar\plugins\colo [0.3.1dev]
  explorer C:\Program Files (x86)\Bazaar\plugins\explorer [1.2.1]
  extmerge C:\Users\fergs\AppData\Roaming\bazaar\2.0\plugins\extmerge [unknown]
  fastimport C:\Program Files (x86)\Bazaar\plugins\fastimport [0.12.0dev]
  launchpad C:\Program Files (x86)\Bazaar\plugins\launchpad [2.4.2]
  loom C:\Program Files (x86)\Bazaar\plugins\loom [2.2.1dev]
  mysql_plugins C:\Users\fergs\AppData\Roaming\bazaar\2.0\plugins\mysql_plugins [0.4.4]
  netrc_credential_store C:\Program Files (x86)\Bazaar\plugins\netrc_credential_store [2.4.2]
  news_merge C:\Program Files (x86)\Bazaar\plugins\news_merge [2.4.2]
  pipeline C:\Program Files (x86)\Bazaar\plugins\pipeline [1.1.0]
  qbzr C:\Program Files (x86)\Bazaar\plugins\qbzr [0.21.1]
  rewrite C:\Program Files (x86)\Bazaar\plugins\rewrite [0.6.3dev]
  svn C:\Program Files (x86)\Bazaar\plugins\svn [1.1.0]
  upload C:\Program Files (x86)\Bazaar\plugins\upload [1.0.1dev]
  xmloutput C:\Program Files (x86)\Bazaar\plugins\xmloutput [0.8.7]

The corrupted contents of qbzr.conf are a chunk of .NET Fusion log viewer (how did they output end up there?):

<meta http-equiv="Content-Type" content="charset=unicode-1-1-utf-8"><!-- saved from url=(0015)assemblybinder: --><html><pre>
*** Assembly Binder Log Entry (7/8/2012 ...