You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2013/05/11 00:19:56 UTC
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
On 05/10/2013 04:51 PM, Daniel Shahaf wrote:
> For issue #4340, we decided to block filenames containing \n in FSFS and
> filenames containing control characters in libsvn_repos. (I agree with
> that decision.)
>
> However, those changes are now nominated for backport towards 1.7.10 in
> 1.7.x/STATUS, and I voted -0 on them, saying that "[the new] libsvn_repos
> restrictions [are] not suitable for introduction in a patch release" ---
> that is, that libsvn_repos-1.7.10 should not forbid creating fspaths that
> contain control characters, because libsvn_repos-1.7.9 doesn't.
>
> Stefan asked to start a thread about that -0 vote, so here it is :)
If you look at the "general idea" behind our versioning guidelines[1], the
one relevant goal of the three presented is this one:
"Upgrading/downgrading between different patch releases in the same
MAJOR.MINOR line never breaks code."
So long as we don't introduce new APIs or break the calling conventions of
existing ones to accomplish this change, disallowing certain characters in
repository paths does not affect the upgrade/downgrade-ability of the
codebase. Further, we're merely guarding our entry points against these
troublesome paths, not croaking when we find them already in the repository.
As such, it's *not* the case that users find themselves with a new freedom
in 1.7.10 that, if exercised, will cause a downgrade to 1.7.9 to render
their repositories inaccessible.
In summary: I think the backport is fine.
Concession: It could be argued that the no-"\n"-in-FSFS change is
independent from the no-control-chars-in-libsvn_repos change. The former is
trivially defensible as a bugfix to guard against a demonstrable data
storage problem. The latter change is, IMO, the only possible controversial
one for a patch release. While I think both backports are fine, I wouldn't
strongly defend the latter.
-- C-Mike
[1]
http://subversion.apache.org/docs/community-guide/releasing.html#release-compat
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 10, 2013 at 06:19:56PM -0400, C. Michael Pilato wrote:
> In summary: I think the backport is fine.
Mike, you still have a -1 recorded in STATUS, which will formally
prevent the change from being backported.
It looks like 1.7.10 is at least a week away still, but I'm mentioning
this now because I'd like to avoid not being able to ship the fix in 1.7.10.
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, May 11, 2013 at 3:24 PM, Stefan Sperling <st...@elego.de> wrote:
> On Sat, May 11, 2013 at 03:13:30PM +0200, Stefan Fuhrmann wrote:
> > You may have gotten lucky
> > if the newline was at the very end of the path,
>
> No, not at all. See the original problem report which was about
> such a case: http://svn.haxx.se/dev/archive-2013-03/0415.shtml
>
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
Posted by Stefan Sperling <st...@elego.de>.
On Sat, May 11, 2013 at 04:31:52PM +0200, Stefan Fuhrmann wrote:
> First off, I'm +1 on fixing that in 1.7.x.
>
> But you omitted my astrology reference in the citation.
:)
> FSFS stores paths in noderev records and changed path
> lists. The respective parsers are somewhat forgiving:
>
> * Empty lines at the end of change path lists get ignored
> * Changed path lists will only be read for "loggy" (log, merge)
> and "dumpy" (dump, verify) operations. They are not
> required for e.g. checkouts.
> * The path element in a noderev record is the last mandatory
> one - the following copyfrom and minfo-* props are optional.
> The parser will simply stop at a newline.
>
> So, committing file paths that end with a newline and
> with just one file / rev, chances are that the corruption
> will only surface in an inconsistent path name (reported
> w/o newline by change lists and noderev but seen with
> newline in the directory entry).
Yes, that's pretty much what happened in the case I observed.
As detailed in the report, 'svnadmin verify' was able to spot the
corrupt node-rev entry, but wasn't able to spot the corrupt
changed-paths list.
> Getting newlines at the end of a file name is most likely
> due to some automated process / script not properly
> splitting its input stream. And those workflows might
> accomplish their goals despite corrupting the repo.
That's true. It depends on the case at hand. But 'svnadmin verify'
will hopefully spot the corrupt revision in any case (but see also
http://subversion.tigris.org/issues/show_bug.cgi?id=4343).
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, May 11, 2013 at 3:24 PM, Stefan Sperling <st...@elego.de> wrote:
> On Sat, May 11, 2013 at 03:13:30PM +0200, Stefan Fuhrmann wrote:
> > You may have gotten lucky
> > if the newline was at the very end of the path,
>
> No, not at all. See the original problem report which was about
> such a case: http://svn.haxx.se/dev/archive-2013-03/0415.shtml
>
First off, I'm +1 on fixing that in 1.7.x.
But you omitted my astrology reference in the citation.
FSFS stores paths in noderev records and changed path
lists. The respective parsers are somewhat forgiving:
* Empty lines at the end of change path lists get ignored
* Changed path lists will only be read for "loggy" (log, merge)
and "dumpy" (dump, verify) operations. They are not
required for e.g. checkouts.
* The path element in a noderev record is the last mandatory
one - the following copyfrom and minfo-* props are optional.
The parser will simply stop at a newline.
So, committing file paths that end with a newline and
with just one file / rev, chances are that the corruption
will only surface in an inconsistent path name (reported
w/o newline by change lists and noderev but seen with
newline in the directory entry).
Getting newlines at the end of a file name is most likely
due to some automated process / script not properly
splitting its input stream. And those workflows might
accomplish their goals despite corrupting the repo.
-- Stefan^2.
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
Posted by Stefan Sperling <st...@elego.de>.
On Sat, May 11, 2013 at 03:13:30PM +0200, Stefan Fuhrmann wrote:
> You may have gotten lucky
> if the newline was at the very end of the path,
No, not at all. See the original problem report which was about
such a case: http://svn.haxx.se/dev/archive-2013-03/0415.shtml
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, May 11, 2013 at 1:05 AM, Daniel Shahaf <da...@elego.de> wrote:
> C. Michael Pilato wrote on Fri, May 10, 2013 at 18:19:56 -0400:
> > On 05/10/2013 04:51 PM, Daniel Shahaf wrote:
> > > For issue #4340, we decided to block filenames containing \n in FSFS
> and
> > > filenames containing control characters in libsvn_repos. (I agree with
> > > that decision.)
> > >
> > > However, those changes are now nominated for backport towards 1.7.10 in
> > > 1.7.x/STATUS, and I voted -0 on them, saying that "[the new]
> libsvn_repos
> > > restrictions [are] not suitable for introduction in a patch release"
> ---
> > > that is, that libsvn_repos-1.7.10 should not forbid creating fspaths
> that
> > > contain control characters, because libsvn_repos-1.7.9 doesn't.
> > >
> > > Stefan asked to start a thread about that -0 vote, so here it is :)
> >
> > If you look at the "general idea" behind our versioning guidelines[1],
> the
> > one relevant goal of the three presented is this one:
> >
> > "Upgrading/downgrading between different patch releases in the same
> > MAJOR.MINOR line never breaks code."
> >
>
> Agreed.
>
> > So long as we don't introduce new APIs or break the calling conventions
> of
> > existing ones to accomplish this change, disallowing certain characters
> in
> > repository paths does not affect the upgrade/downgrade-ability of the
>
> Disagree. Adding a validation to 1.7.10 that didn't exist in 1.7.9
> means that client code that worked with a 1.7.9 libsvn would stop
> working[1] when run with 1.7.10 libsvn. That is precisely "Upgrading
> within the same minor line never breaks code", isn't it?
>
My understanding is that an newline in a file path will
always corrupt your repository, i.e. it has never been
"working" in a strict sense. You may have gotten lucky
if the newline was at the very end of the path, the stars
were in the right position etc. but chances are that it
would still bite you further down the road.
-- Stefan^2.
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*
RE: forbidding control characters prohibition in libsvn_repos in 1.7.10 (issue #4340)
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:danielsh@elego.de]
> Sent: zaterdag 11 mei 2013 01:06
> To: C. Michael Pilato
> Cc: dev@subversion.apache.org
> Subject: Re: forbidding control characters prohibition in libsvn_repos in
1.7.10
> (issue #4340)
>
> C. Michael Pilato wrote on Fri, May 10, 2013 at 18:19:56 -0400:
> > On 05/10/2013 04:51 PM, Daniel Shahaf wrote:
> > > For issue #4340, we decided to block filenames containing \n in FSFS
and
> > > filenames containing control characters in libsvn_repos. (I agree
with
> > > that decision.)
> > >
> > > However, those changes are now nominated for backport towards 1.7.10
> in
> > > 1.7.x/STATUS, and I voted -0 on them, saying that "[the new]
> libsvn_repos
> > > restrictions [are] not suitable for introduction in a patch release"
---
> > > that is, that libsvn_repos-1.7.10 should not forbid creating fspaths
that
> > > contain control characters, because libsvn_repos-1.7.9 doesn't.
> > >
> > > Stefan asked to start a thread about that -0 vote, so here it is :)
> >
> > If you look at the "general idea" behind our versioning guidelines[1],
the
> > one relevant goal of the three presented is this one:
> >
> > "Upgrading/downgrading between different patch releases in the same
> > MAJOR.MINOR line never breaks code."
> >
>
> Agreed.
>
> > So long as we don't introduce new APIs or break the calling conventions
of
> > existing ones to accomplish this change, disallowing certain characters
in
> > repository paths does not affect the upgrade/downgrade-ability of the
>
> Disagree. Adding a validation to 1.7.10 that didn't exist in 1.7.9
> means that client code that worked with a 1.7.9 libsvn would stop
> working[1] when run with 1.7.10 libsvn. That is precisely "Upgrading
> within the same minor line never breaks code", isn't it?
Taken 100% literally: If downgrading can never break code, we can not
backport any bugfixes. As downgrading to the unfixed version would
reintroduce the bug that we fixed.
(I'm not sure what we could ever backport in that world?)
If you read this as 'the public API can't change in a breaking way' I don't
see a real problem.
Bert
Re: forbidding control characters prohibition in libsvn_repos in
1.7.10 (issue #4340)
Posted by Daniel Shahaf <da...@elego.de>.
C. Michael Pilato wrote on Fri, May 10, 2013 at 18:19:56 -0400:
> On 05/10/2013 04:51 PM, Daniel Shahaf wrote:
> > For issue #4340, we decided to block filenames containing \n in FSFS and
> > filenames containing control characters in libsvn_repos. (I agree with
> > that decision.)
> >
> > However, those changes are now nominated for backport towards 1.7.10 in
> > 1.7.x/STATUS, and I voted -0 on them, saying that "[the new] libsvn_repos
> > restrictions [are] not suitable for introduction in a patch release" ---
> > that is, that libsvn_repos-1.7.10 should not forbid creating fspaths that
> > contain control characters, because libsvn_repos-1.7.9 doesn't.
> >
> > Stefan asked to start a thread about that -0 vote, so here it is :)
>
> If you look at the "general idea" behind our versioning guidelines[1], the
> one relevant goal of the three presented is this one:
>
> "Upgrading/downgrading between different patch releases in the same
> MAJOR.MINOR line never breaks code."
>
Agreed.
> So long as we don't introduce new APIs or break the calling conventions of
> existing ones to accomplish this change, disallowing certain characters in
> repository paths does not affect the upgrade/downgrade-ability of the
Disagree. Adding a validation to 1.7.10 that didn't exist in 1.7.9
means that client code that worked with a 1.7.9 libsvn would stop
working[1] when run with 1.7.10 libsvn. That is precisely "Upgrading
within the same minor line never breaks code", isn't it?
> codebase. Further, we're merely guarding our entry points against these
> troublesome paths, not croaking when we find them already in the repository.
Agreed.
> As such, it's *not* the case that users find themselves with a new freedom
> in 1.7.10 that, if exercised, will cause a downgrade to 1.7.9 to render
> their repositories inaccessible.
>
Agreed again, downgrading would not cause a problem.
> In summary: I think the backport is fine.
>
> Concession: It could be argued that the no-"\n"-in-FSFS change is
> independent from the no-control-chars-in-libsvn_repos change. The former is
> trivially defensible as a bugfix to guard against a demonstrable data
> storage problem. The latter change is, IMO, the only possible controversial
> one for a patch release. While I think both backports are fine, I wouldn't
> strongly defend the latter.
>
I already voted +1 on the "No '\n' in FSFS" change; I only have concerns
about the "No control-chars in libsvn_repos" change.
>
Thanks for the feedback.
Cheers,
Daniel
[1] Specifically: svn_repos_fs_commit_txn() would start returning
an SVN_ERR_FS_PATH_SYNTAX error instead of succeeding.