You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by gs...@apache.org on 2012/03/28 03:51:54 UTC
svn commit: r1306111 - in /subversion/trunk/subversion:
libsvn_ra_svn/client.c svnserve/serve.c
Author: gstein
Date: Wed Mar 28 01:51:54 2012
New Revision: 1306111
URL: http://svn.apache.org/viewvc?rev=1306111&view=rev
Log:
Fix a problem in the svn protocol's get_dir() functionality. The tuple
specification allowed for a NULL created-date, but the client would
segfault with that input (via svn_time_from_string).
This is related to the work in r1296628 and r1305964. I believe this
"happened to work" until r1296628. Before that revision, the value
((time_t) -1) was stored into svn_dirent_t.time which is an apr_time_t
(a 64-bit value, compared to time_t's typical 32 bits). This likely
caused a later check to think a time was present, and for a time
string to be generated (thus: never NULL).
So... when a created-date is missing, we fill in the epoch date.
Hopefully, the client will translate this back to 0, and consider the
date "missing". As a precaution, the client will also check for NULL
to avoid malicious servers passing NULL in the tuple.
Note: I took a brief look at the 1.6.x client, and this failure is
present. Thus, we must pass non-NULL for compatibility purposes, to
avoid older clients from segfaulting.
* subversion/libsvn_ra_svn/client.c:
(ra_svn_get_dir): beware a NULL CDATE value. add commentary.
* subversion/svnserve/serve.c:
(get_dir): tighten the scope of a bunch of variables for clarity.
use actual variables rather than a structure (ie. nix ENTRY). use
the new MISSING_DATE localvar when created-date is missing. leave
commentary for future readers.
Modified:
subversion/trunk/subversion/libsvn_ra_svn/client.c
subversion/trunk/subversion/svnserve/serve.c
Modified: subversion/trunk/subversion/libsvn_ra_svn/client.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/client.c?rev=1306111&r1=1306110&r2=1306111&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/client.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/client.c Wed Mar 28 01:51:54 2012
@@ -1152,7 +1152,18 @@ static svn_error_t *ra_svn_get_dir(svn_r
dirent->size = size;/* FIXME: svn_filesize_t */
dirent->has_props = has_props;
dirent->created_rev = crev;
- SVN_ERR(svn_time_from_cstring(&dirent->time, cdate, pool));
+ /* NOTE: the tuple's format string says CDATE may be NULL. But this
+ function does not allow that. The server has always sent us some
+ random date, however, so this just happens to work. But let's
+ be wary of servers that are (improperly) fixed to send NULL.
+
+ Note: they should NOT be "fixed" to send NULL, as that would break
+ any older clients which received that NULL. But we may as well
+ be defensive against a malicous server. */
+ if (cdate == NULL)
+ dirent->time = 0;
+ else
+ SVN_ERR(svn_time_from_cstring(&dirent->time, cdate, pool));
dirent->last_author = cauthor;
apr_hash_set(*dirents, name, APR_HASH_KEY_STRING, dirent);
}
Modified: subversion/trunk/subversion/svnserve/serve.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/serve.c?rev=1306111&r1=1306110&r2=1306111&view=diff
==============================================================================
--- subversion/trunk/subversion/svnserve/serve.c (original)
+++ subversion/trunk/subversion/svnserve/serve.c Wed Mar 28 01:51:54 2012
@@ -1470,9 +1470,9 @@ static svn_error_t *get_dir(svn_ra_svn_c
apr_array_header_t *params, void *baton)
{
server_baton_t *b = baton;
- const char *path, *full_path, *file_path, *cdate;
+ const char *path, *full_path;
svn_revnum_t rev;
- apr_hash_t *entries, *props = NULL, *file_props;
+ apr_hash_t *entries, *props = NULL;
apr_hash_index_t *hi;
svn_fs_root_t *root;
apr_pool_t *subpool;
@@ -1548,6 +1548,9 @@ static svn_error_t *get_dir(svn_ra_svn_c
/* Fetch the directory entries if requested and send them immediately. */
if (want_contents)
{
+ /* Use epoch for a placeholder for a missing date. */
+ const char *missing_date = svn_time_to_cstring(0, pool);
+
SVN_CMD_ERR(svn_fs_dir_entries(&entries, root, full_path, pool));
/* Transform the hash table's FS entries into dirents. This probably
@@ -1557,9 +1560,15 @@ static svn_error_t *get_dir(svn_ra_svn_c
{
const char *name = svn__apr_hash_index_key(hi);
svn_fs_dirent_t *fsent = svn__apr_hash_index_val(hi);
+ const char *file_path;
- svn_dirent_t entry;
- memset(&entry, 0, sizeof(entry));
+ /* The fields in the entry tuple. */
+ svn_node_kind_t entry_kind = svn_node_none;
+ svn_filesize_t entry_size = 0;
+ svn_boolean_t has_props = FALSE;
+ svn_revnum_t created_rev = 0; /* ### SVN_INVALID_REVNUM */
+ const char *cdate = NULL;
+ const char *last_author = NULL;
svn_pool_clear(subpool);
@@ -1569,41 +1578,49 @@ static svn_error_t *get_dir(svn_ra_svn_c
continue;
if (dirent_fields & SVN_DIRENT_KIND)
- entry.kind = fsent->kind;
+ entry_kind = fsent->kind;
if (dirent_fields & SVN_DIRENT_SIZE)
- if (entry.kind != svn_node_dir)
- SVN_CMD_ERR(svn_fs_file_length(&entry.size, root, file_path,
+ if (entry_kind != svn_node_dir)
+ SVN_CMD_ERR(svn_fs_file_length(&entry_size, root, file_path,
subpool));
if (dirent_fields & SVN_DIRENT_HAS_PROPS)
{
+ apr_hash_t *file_props;
+
/* has_props */
SVN_CMD_ERR(svn_fs_node_proplist(&file_props, root, file_path,
subpool));
- entry.has_props = (apr_hash_count(file_props) > 0);
+ has_props = (apr_hash_count(file_props) > 0);
}
- cdate = NULL;
if ((dirent_fields & SVN_DIRENT_LAST_AUTHOR)
|| (dirent_fields & SVN_DIRENT_TIME)
|| (dirent_fields & SVN_DIRENT_CREATED_REV))
{
/* created_rev, last_author, time */
- SVN_CMD_ERR(svn_repos_get_committed_info(&entry.created_rev,
+ SVN_CMD_ERR(svn_repos_get_committed_info(&created_rev,
&cdate,
- &entry.last_author,
+ &last_author,
root,
file_path,
subpool));
}
+ /* The client does not properly handle a missing CDATE. For
+ interoperability purposes, we must fill in some junk.
+
+ See libsvn_ra_svn/client.c:ra_svn_get_dir() */
+ if (cdate == NULL)
+ cdate = missing_date;
+
/* Send the entry. */
SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "cwnbr(?c)(?c)", name,
- svn_node_kind_to_word(entry.kind),
- (apr_uint64_t) entry.size,
- entry.has_props, entry.created_rev,
- cdate, entry.last_author));
+ svn_node_kind_to_word(entry_kind),
+ (apr_uint64_t) entry_size,
+ has_props, created_rev,
+ cdate, last_author));
}
svn_pool_destroy(subpool);
}
Re: svn commit: r1306111 - in /subversion/trunk/subversion:
libsvn_ra_svn/client.c svnserve/serve.c
Posted by Greg Stein <gs...@gmail.com>.
On Mar 28, 2012 4:23 AM, "Daniel Shahaf" <d....@daniel.shahaf.name> wrote:
>
> gstein@apache.org wrote on Wed, Mar 28, 2012 at 01:51:54 -0000:
> > Author: gstein
> > Date: Wed Mar 28 01:51:54 2012
> > New Revision: 1306111
> >
> > URL: http://svn.apache.org/viewvc?rev=1306111&view=rev
> > Log:
> > Fix a problem in the svn protocol's get_dir() functionality. The tuple
> > specification allowed for a NULL created-date, but the client would
> > segfault with that input (via svn_time_from_string).
> >
>
> Backport?
For client resilience, sure. But very, very low footprint/issue. IMO, not a
release driver, but something to get caught up into a release.
>
> > - svn_dirent_t entry;
> > - memset(&entry, 0, sizeof(entry));
> > + /* The fields in the entry tuple. */
> > + svn_node_kind_t entry_kind = svn_node_none;
> > + svn_filesize_t entry_size = 0;
> > + svn_boolean_t has_props = FALSE;
> > + svn_revnum_t created_rev = 0; /* ### SVN_INVALID_REVNUM */
> > + const char *cdate = NULL;
> > + const char *last_author = NULL;
>
> Unrelated change.
Right. Agreed. Only client.c is interesting for a backport.
Cheers,
-g
Re: svn commit: r1306111 - in /subversion/trunk/subversion:
libsvn_ra_svn/client.c svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
gstein@apache.org wrote on Wed, Mar 28, 2012 at 01:51:54 -0000:
> Author: gstein
> Date: Wed Mar 28 01:51:54 2012
> New Revision: 1306111
>
> URL: http://svn.apache.org/viewvc?rev=1306111&view=rev
> Log:
> Fix a problem in the svn protocol's get_dir() functionality. The tuple
> specification allowed for a NULL created-date, but the client would
> segfault with that input (via svn_time_from_string).
>
Backport?
> - svn_dirent_t entry;
> - memset(&entry, 0, sizeof(entry));
> + /* The fields in the entry tuple. */
> + svn_node_kind_t entry_kind = svn_node_none;
> + svn_filesize_t entry_size = 0;
> + svn_boolean_t has_props = FALSE;
> + svn_revnum_t created_rev = 0; /* ### SVN_INVALID_REVNUM */
> + const char *cdate = NULL;
> + const char *last_author = NULL;
Unrelated change.
Re: svn commit: r1306111 - in /subversion/trunk/subversion:
libsvn_ra_svn/client.c svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
gstein@apache.org wrote on Wed, Mar 28, 2012 at 01:51:54 -0000:
> Author: gstein
> Date: Wed Mar 28 01:51:54 2012
> New Revision: 1306111
>
> URL: http://svn.apache.org/viewvc?rev=1306111&view=rev
> Log:
> Fix a problem in the svn protocol's get_dir() functionality. The tuple
> specification allowed for a NULL created-date, but the client would
> segfault with that input (via svn_time_from_string).
>
Backport?
> - svn_dirent_t entry;
> - memset(&entry, 0, sizeof(entry));
> + /* The fields in the entry tuple. */
> + svn_node_kind_t entry_kind = svn_node_none;
> + svn_filesize_t entry_size = 0;
> + svn_boolean_t has_props = FALSE;
> + svn_revnum_t created_rev = 0; /* ### SVN_INVALID_REVNUM */
> + const char *cdate = NULL;
> + const char *last_author = NULL;
Unrelated change.