svn-3400

Version:

1.5.x

Bug Link:

http://subversion.tigris.org/issues/show_bug.cgi?id=3400

http://svn.haxx.se/dev/archive-2009-04/0466.shtml

Patch Link:

http://svn.apache.org/viewvc?view=revision&revision=879033

Symptom:

“svn export” won’t complain on an invalid revision but give a different valid revision instead.

How it is diagnosed:

reproduced!

How to reproduce:

/*svn export will accept it but export a different revision*/

$./svn export -r 1000000 http://llvm.org/svn/llvm-project/llvm/trunk/include/llvm/ADT/  foo

A foo
A foo/ScopedHashTable.h
[...]
A foo/APFloat.h
Exported revision 123337. <--- svn exported this wrong revision!

/*svn checkout will complain on an invalid revision.*/

$./svn checkout -r 1000000 http://llvm.org/svn/llvm-project/llvm/trunk/include/llvm/ADT/ llvm

svn: No such revision 1000000

Root Cause:

Brief:

When they are checking the input revision number and found it is larger than the largest valid one (youngest revision number), they change the program’s semantic behavior and convert the invalid revision to the latest revision silently.

Detail:

Basically, the correct semantic should be: if there’s no such revision, just report error, but the buggy case, they checked whether user’s revision number is valid or not, and if it is invalid (too large), they want to “predict” user’s intention supposing that user’s supplying a very high invalid revision may just want to manipulate the latest revision but don’t know the exact revision number.

In other words, they already noticed the abnormal or exceptional behavior (user input revision number is larger than the youngest valid revision), and they tried to handle it themselves.

What we propose is: no matter what you do, if you notice an exception, LOG IT!

Subversion uses svn_client__ra_session_from_path() to parse the

revision conversion. This in turn calls svn_client__get_revision_number()

which has this code fragment:

   /* Here, they are doing input check on the revision number: revnum holds

       the user input revision number: 1000000. The last condition:

       *revnum > *youngest_rev, is to check if the user input revision number

       is larger than the youngest one -- normally it should not if it is a valid

       revision number, and it is abnormal if it is larger than the youngest --

       indicating an invalid revision number!!! */

   /* To handle it, they simply assigned the revision number to the youngest

       one!*/  

  if (youngest_rev

/*the patch: the revision kind is svn_opt_revision_number, by adding this restriction, it will not fall here*/

+      && (revision->kind == svn_opt_revision_head
+          || revision->kind == svn_opt_revision_date)

/*here, SVN_IS_VALID_REVNUM macro only checks if the revision number is >=0*/

      && SVN_IS_VALID_REVNUM(*youngest_rev)

      && SVN_IS_VALID_REVNUM(*revnum)

      /* *revnum == 1000000, *youngest_rev == 12337*/

      && (*revnum > *youngest_rev))

/*

the buggy case will change revnum to the latest revision, if the requested revision is larger than the latest revision, should change it to print an error message.

*/

        *revnum = *youngest_rev;

/*So essentially, we should remove this hack. And give an error message instead. But the developers are worried it may have concern that changing it has a lot of impact on the rest of the code for users whomight expect the current behavior, they figure out another work around, but it has the same is the same to ensure not change the revnum.

*/

The reason why svn checkout complain about the high revision is tricky, the whole execution path of these two sub commands are very different. When svn checkout calls the svn_client__get_revision_number it somehow get around this complicated if statement, and the requested revision(1000000) is not set to the youngest_rev(123337). And when it negotiate with the repository, it cannot fetch the requested revision and so complains. But svn export’s way of calling this function unfortunately falls into the if statement and reset the requested revision to youngest one before it sends the export request to repository:

svn_error_t *

svn_client__get_revision_number(svn_revnum_t *revnum,

                     svn_revnum_t *youngest_rev...const svn_opt_revision_t *revision..)

{

  switch (revision->kind)

    {

    case svn_opt_revision_unspecified:

     …

    /*svn co ”second” time to get the requested revision*/

    case svn_opt_revision_number:

      *revnum = revision->value.number; //get set to 1000000

      break;

   /*svn co first time falls here to get the head*/

    case svn_opt_revision_head:

      if (youngest_rev && SVN_IS_VALID_REVNUM(*youngest_rev))

        {

          *revnum = *youngest_rev;

        }

      else

        {//svn co first time falls here

          if (! ra_session)

            return svn_error_create(SVN_ERR_CLIENT_RA_ACCESS_REQUIRED,

                                    NULL, NULL);

          SVN_ERR(svn_ra_get_latest_revnum(ra_session, revnum, pool));

          if (youngest_rev)

            *youngest_rev = *revnum;//get set to 123337

        }

      break;

    case svn_opt_revision_committed:

    case svn_opt_revision_working:

    case svn_opt_revision_base:

    case svn_opt_revision_previous:

          ...

    case svn_opt_revision_date:

...

    default:

      return svn_error_createf(SVN_ERR_CLIENT_BAD_REVISION, NULL,

                               _("Unrecognized revision type requested for "

                                 "'%s'"),

                               svn_path_local_style(path, pool));

    }

/*

svn co fisrt time yougnest_rev and revnum are the same 123337, won’t fall here;

svn co ”second” time yougnest_rev is set to null, wont’t fall here. thus has no effect to svn co

*/

  if (youngest_rev

      && SVN_IS_VALID_REVNUM(*youngest_rev)

      && SVN_IS_VALID_REVNUM(*revnum)

      && (*revnum > *youngest_rev))

    *revnum = *youngest_rev;

  return SVN_NO_ERROR;

}

---- omit

Where the error message of the SVN checkout is printed?

Although the source file has several function that can print this “No such revision” error message, it seems they’re not responsible for printing it(I set up breakpoint for all of them, but none is caught). From the stack trace in maintainer mode, it’s subversion/libsvn_ra_neon/util.c:711: (apr_err=160006). I doubt it’s an error message from the repository server in the negotiation.

Failure symptom category

Accepting invalid input

Is there any log message?

No

Can we anticipate error?

Input check -- they are checking the input revision number (1000000) whether it is larger than the largest valid one. When it failed, they should LOG!

How do we handle it?

invariant checking?

key variable value: *revnum, *youngest_rev