Re: [PATCH] Was: Re: Problem in 1.3.0 RC-6

From: Paul Burba <paulb_at_softlanding.com>
Date: 2005-12-17 02:24:25 CET

-----Philip Martin <philip@codematters.co.uk> wrote: -----

>> Hi Philip,
>>
>> I'm not sure I understand your concern. Is it that apr_pstrdup
>shouldn't 
>> be passed a null value?
>
>That was my concern.
>
>>  Or is it something that might occur later if 
>> statstruct->ood_last_cmt_author is set to null? Or something else
>
>> entirely?
>>
>> On windows at least, if you pass apr_pstrdup null, it just returns
>null. 
>> Is this not a safe cross-platform assumption?
>
>The documentation for apr_pstrdup doesn't say that NULL is a valid
>input, but it appears that the code accepts it. I don't know
>whether
>that makes it safe or not. 

>Why does the code have an 'if' check for b->url but not for
>b->ood_last_cmt_author?

I'm not sure what the original thought on that was.

 

I took a casual look at the use of apr_pstrdup in other files and *not* checking for nullness seems the rule.  However I can't help but think that it would be technically correct to check, since, as you point out, the APR docs don't promise anything.  So in that spirit here is a slightly revised patch.

 

Paul B.

 

[[[
Follow-up to r16344 and r16494.

Fix potential corruption of out of date info in svn_wc_status2_t.

* subversion/libsvn_wc/status.c
  (tweak_statushash): When copying out of date string info to the
   svn_wc_status2_t structure, use apr_pstrdup rather than assigning
   directly to the file/dir baton since the baton will be destroyed.
]]]

Index: subversion/libsvn_wc/status.c
===================================================================
--- subversion/libsvn_wc/status.c       (revision 17694)
+++ subversion/libsvn_wc/status.c       (working copy)
@@ -1054,7 +1054,7 @@
    {
      struct dir_baton *b = baton;
      if (b->url)
-        statstruct->url = b->url;
+        statstruct->url = apr_pstrdup (pool, b->url);
      statstruct->ood_kind = b->ood_kind;
      /* The last committed rev, date, and author for deleted items
         isn't available. */
@@ -1062,18 +1062,20 @@
        {
          statstruct->ood_last_cmt_rev = b->ood_last_cmt_rev;
          statstruct->ood_last_cmt_date = b->ood_last_cmt_date;
-          statstruct->ood_last_cmt_author = b->ood_last_cmt_author;
+          if (b->ood_last_cmt_author)

+            statstruct->ood_last_cmt_author =
+              apr_pstrdup (pool, b->ood_last_cmt_author);
        }
    }
  else
    {
      struct file_baton *b = baton;
      if (b->url)
-        statstruct->url = b->url;

+        statstruct->url = apr_pstrdup (pool, b->url);
      statstruct->ood_last_cmt_rev = b->ood_last_cmt_rev;
      statstruct->ood_last_cmt_date = b->ood_last_cmt_date;
      statstruct->ood_kind = b->ood_kind;
-      statstruct->ood_last_cmt_author = b->ood_last_cmt_author;
+      if (b->ood_last_cmt_author)

+        statstruct->ood_last_cmt_author =

+          apr_pstrdup (pool, b->ood_last_cmt_author);
    }
  return SVN_NO_ERROR;
}

_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
--------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org For additional commands, e-mail: dev-help@subversion.tigris.org Received on Sat Dec 17 02:25:09 2005