svn-3666

Version:

1.7.0

Bug Link:

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

Patch Link:

source code patch: http://svn.apache.org/viewvc?diff_format=h&view=revision&revision=1031165

reproduction: http://subversion.tigris.org/nonav/issues/showattachment.cgi/1114/issue-3666-recipe.sh

Symptom:

‘svn blame’ leads to ‘asseration fail’ in libsvn_client/blame.c with the message ‘Assertion frb->last_filename == NULL’.

 

*background: ‘svn blame/annotate/praise/ann’ outputs the content of specified files/URLs with revision and author information in-line.

How it is diagnosed:

reproduced

How to diagnose:

Run http://subversion.tigris.org/nonav/issues/showattachment.cgi/1114/issue-3666-recipe.sh

ð  Executes ‘svn blame –g –r4:5’, which requests the info with the merged information(annotation ‘G’) in the given revision range.

 

expected result:

 

At revision 5.

G          -              - foo

           5          mmlee bar

if without -g

4 mmlee foo

5 mmlee bar

 

(c.f. ‘G’ means ‘merged’.)

 

*Buggy Result:

 

[debugging message] subversion/libsvn_fs_fs/tree.c:663: (apr_err=235000)

[error message] svn: In file 'subversion/libsvn_client/blame.c' line 487: assertion failed (frb->last_filename == NULL)

./issue-3666-recipe.sh: line 28:  1614 Aborted                     ${SVN} annotate -g -r 4:5 ${WC}/trunk/file

Root Cause:

Brief:

The previous condition check is too strict. They haven’t considered the case of tracking merged revisions.

Detail:

--- subversion/trunk/subversion/libsvn_client/blame.c           2010/11/04 19:16:26              1031164

@@ -490,8 +492,10 @@ file_rev_handler(void *baton, const char

 

   /* Here, the developers thought frb->last_filename should be NULL here,

       since this is before they start the ‘blame’ process. however, if the user is

       using with “-g”, which is “merge”, then only under this condition

       frb->last_filename is not NULL, since it is from some old revision merged. */

   if (revnum < frb->start_rev)

         {

-          /* We shouldn't get more than one revision before start. */

-      SVN_ERR_ASSERT(frb->last_filename == NULL);

+          /* We shouldn't get more than one revision before the starting

+             revision (unless of including merged revisions). */

+      SVN_ERR_ASSERT((frb->last_filename == NULL)

+                         || frb->include_merged_revisions);

 

 /* when we track merged revisions, they can have ‘frb->last_filename’, which means the name of file containing the previous revision of the file */

 

           /* The file existed before start_rev; generate no blame info for

              lines from this revision (or before). */

#define SVN_ERR_ASSERT(expr)                                            \

  do {                                                                  \

    if (!(expr))                                                        \

    SVN_ERR(svn_error__malfunction(TRUE, __FILE__, __LINE__, #expr)); \

  } while (0)

/* svn_error__malfunction eventually will call svn_error_abort_on_malfunction

    , which will abort!  */

svn_error_t *

svn_error_abort_on_malfunction(svn_boolean_t can_return,

                               const char *file, int line,

                               const char *expr)

{

  svn_error_t *err = svn_error_raise_on_malfunction(TRUE, file, line, expr);

  svn_handle_error2(err, stderr, FALSE, "svn: ");

  abort();

  return err;  /* Not reached. */

}

Failure symptom category

refuse valid input, early termination

Is there any log message?

Yes

How can we automatically insert the log message?

Log before abort (abnormal exit). Of course we are relying on the programmers’ domain knowledge to put a check there in the first place (frb->last_filename == NULL), but we argue that once there is such check exists and it aborts, it SHOULD ALWAYS HAVE AN ERROR MESSAGE!

Here, the developer thinks the frb->last_filename must be NULL, and they used:

SVN_ERR_ASSERT (frb->last_filename);

which will abort. Essentially, they are checking the condition (which is domain specific), and abort.

It is a good practice to put a log message before they abort. Otherwise, just imagine if there is no log message in SVN_ERR_ASSERT, they will fail silently without knowing where it died. 

#define SVN_ERR_ASSERT(expr)                                            \

  do {                                                                  \

    if (!(expr))                                                        \

    SVN_ERR(svn_error__malfunction(TRUE, __FILE__, __LINE__, #expr)); \

  } while (0)

/* svn_error__malfunction eventually will call svn_error_abort_on_malfunction

    , which will abort!  */

svn_error_t *

svn_error_abort_on_malfunction(svn_boolean_t can_return,

                               const char *file, int line,

                               const char *expr)

{

  svn_error_t *err = svn_error_raise_on_malfunction(TRUE, file, line, expr);

  svn_handle_error2(err, stderr, FALSE, "svn: ");

  abort();

  return err;  /* Not reached. */

}