You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2006/02/22 00:24:22 UTC
Checking for NULL from svn_wc_entry
So after the problem with the crash involving diff, nonrecursive
checkouts, and deleted directories, I decided to investigate how we
deal with possible NULL entries returned by svn_wc_entry. The return
value of this function is based on the contents of the .svn/entries
file, so it's possible for crashes to result from users accidentally
mangling those files, which is bad.
There are approximately 20 places in libsvn_wc (I haven't looked in
other libraries) that have somewhat questionable handling of the
result of that function. I've got a patch here that fixes a bunch of
them, but I'm not the best with libsvn_wc, so I wanted to run it by
someone else.
There's also a question in 3 of the cases, the check might not be
needed at all. If we're using svn_wc_adm_access_path to get the path
argument, does that mean the entry will already be cached and thus
can't be NULL? If so, the parts marked with /* XXX ??? */ comments in
libsvn_wc/log.c are safe, otherwise they need attention.
As far as this patch goes, there are 16 places where it inserts checks
that return error if the entry is NULL, and 1 place where it fixes the
check that was there to refer to the correct variable. It makes it
through make check, but I haven't done a whole lot else with it at
this point.
-garrett
Re: Checking for NULL from svn_wc_entry
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 2/22/06, Philip Martin <ph...@codematters.co.uk> wrote:
> "Garrett Rooney" <ro...@electricjellyfish.net> writes:
>
> > There's also a question in 3 of the cases, the check might not be
> > needed at all. If we're using svn_wc_adm_access_path to get the path
> > argument, does that mean the entry will already be cached and thus
> > can't be NULL?
>
> I believe every access baton must have a corresponding entry, so it
> can't be NULL.
Makes sense to me.
> > If so, the parts marked with /* XXX ??? */ comments in
> > libsvn_wc/log.c are safe, otherwise they need attention.
> >
> > As far as this patch goes, there are 16 places where it inserts checks
> > that return error if the entry is NULL, and 1 place where it fixes the
> > check that was there to refer to the correct variable. It makes it
> > through make check, but I haven't done a whole lot else with it at
> > this point.
> >
[snip nice long review]
Thanks for the review, I think the version I committed in r18587
should take all your comments into account, although I didn't tackle
the "maybe we should be passing an entry here" part.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Checking for NULL from svn_wc_entry
Posted by Philip Martin <ph...@codematters.co.uk>.
"Garrett Rooney" <ro...@electricjellyfish.net> writes:
> There's also a question in 3 of the cases, the check might not be
> needed at all. If we're using svn_wc_adm_access_path to get the path
> argument, does that mean the entry will already be cached and thus
> can't be NULL?
I believe every access baton must have a corresponding entry, so it
can't be NULL.
> If so, the parts marked with /* XXX ??? */ comments in
> libsvn_wc/log.c are safe, otherwise they need attention.
>
> As far as this patch goes, there are 16 places where it inserts checks
> that return error if the entry is NULL, and 1 place where it fixes the
> check that was there to refer to the correct variable. It makes it
> through make check, but I haven't done a whole lot else with it at
> this point.
>
> -garrett
>
> Index: subversion/libsvn_wc/diff.c
> ===================================================================
> --- subversion/libsvn_wc/diff.c (revision 18539)
> +++ subversion/libsvn_wc/diff.c (working copy)
> @@ -55,6 +55,7 @@
> #include "props.h"
> #include "adm_files.h"
>
> +#include "svn_private_config.h"
>
> /*-------------------------------------------------------------------------*/
> /* A little helper function.
> @@ -1732,6 +1733,10 @@
> SVN_ERR(svn_wc_adm_probe_retrieve(&adm_access, anchor, target_path,
> eb->pool));
> SVN_ERR(svn_wc_entry(&entry, target_path, adm_access, FALSE, eb->pool));
> + if (! entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(target_path, pool));
>
> if (entry->kind == svn_node_dir)
> b = make_dir_baton(target_path, NULL, eb, FALSE, eb->pool);
> Index: subversion/libsvn_wc/adm_crawler.c
> ===================================================================
> --- subversion/libsvn_wc/adm_crawler.c (revision 18538)
> +++ subversion/libsvn_wc/adm_crawler.c (working copy)
> @@ -99,7 +99,10 @@
> const svn_wc_entry_t *entry;
>
> SVN_ERR(svn_wc_entry(&entry, file_path, adm_access, FALSE, pool));
> - assert(entry != NULL);
> + if (! entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(file_path, pool));
>
> SVN_ERR(svn_io_set_file_affected_time(entry->cmt_date,
> file_path, pool));
This is a static function and the callers have already verified that
file_path has a non-NULL entry. Perhaps the callers should be passing
an svn_wc_entry_t?
> @@ -349,6 +352,11 @@
> this_full_path, iterpool));
> SVN_ERR(svn_wc_entry(&subdir_entry, this_full_path, subdir_access,
> TRUE, iterpool));
> + if (! subdir_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(this_full_path,
> + pool));
There is a call to svn_wc_adm_retrieve passing this_full_path so
subdir_entry cannot be NULL.
>
> if (report_everything)
> {
> @@ -463,10 +471,14 @@
> base_rev = entry->revision;
> if (base_rev == SVN_INVALID_REVNUM)
> {
> - SVN_ERR(svn_wc_entry(&parent_entry,
> - svn_path_dirname(path, pool),
> - adm_access,
> - FALSE, pool));
> + const char *dirname = svn_path_dirname(path, pool);
> +
> + SVN_ERR(svn_wc_entry(&parent_entry, dirname, adm_access, FALSE, pool));
> + if (! parent_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(dirname, pool));
> +
> base_rev = parent_entry->revision;
> }
>
> @@ -740,7 +752,11 @@
> Otherwise we could send corrupt data and never know it. */
> const svn_wc_entry_t *ent;
> SVN_ERR(svn_wc_entry(&ent, path, adm_access, FALSE, pool));
> -
> + if (! ent)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(path, pool));
> +
> /* For backwards compatibility, no checksum means assume a match. */
> if (ent->checksum)
> {
> Index: subversion/libsvn_wc/log.c
> ===================================================================
> --- subversion/libsvn_wc/log.c (revision 18538)
> +++ subversion/libsvn_wc/log.c (working copy)
> @@ -734,7 +734,7 @@
> if (err)
> signal_error(loggy, err);
>
> - if (! entry)
> + if (! tfile_entry)
> return SVN_NO_ERROR;
>
> err = svn_wc__prop_path(&pfile, tfile, tfile_entry->kind, FALSE,
> @@ -1018,6 +1018,8 @@
> svn_wc_adm_access_path(loggy->adm_access),
> loggy->adm_access,
> TRUE, pool));
> + /* XXX ??? */
> +
Using an access baton path so entry can't be NULL.
> if (new_rev > parentry->revision)
> {
> /* ...then the parent's revision is now officially a
> @@ -1521,6 +1523,8 @@
> svn_wc_adm_access_path(adm_access), adm_access,
> FALSE, pool));
>
> + /* XXX ??? */
> +
Using an access baton path so entry can't be NULL.
> /* Blow away the entire directory, and all those below it too. */
> err = svn_wc_remove_from_revision_control(adm_access,
> SVN_WC_ENTRY_THIS_DIR,
> @@ -1541,7 +1545,9 @@
> svn_path_split(svn_wc_adm_access_path(adm_access), &parent, &bname, pool);
> SVN_ERR(svn_wc_adm_retrieve(&parent_access, adm_access, parent, pool));
> SVN_ERR(svn_wc_entry(&parent_entry, parent, parent_access, FALSE, pool));
> -
> +
> + /* XXX ??? */
> +
There is a call to svn_wc_adm_retrieve passing parent so parent_entry
cannot be NULL.
> if (thisdir_entry->revision > parent_entry->revision)
> {
> tmp_entry.kind = svn_node_dir;
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 18538)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -1000,6 +1000,10 @@
> const svn_wc_entry_t *ent;
>
> SVN_ERR(svn_wc_entry(&ent, path, adm_access, FALSE, pool));
> + if (! ent)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(path, pool));
>
> if (url)
> *url = apr_pstrdup(pool, ent->url);
> @@ -1168,7 +1172,10 @@
> the ancestor path out of there. */
> SVN_ERR(svn_wc_entry(&p_entry, parent_dir, parent_access, FALSE,
> pool));
> -
> + if (! p_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(parent_dir, pool));
> /* Derive the parent path for our new addition here. */
> new_url = svn_path_url_add_component(p_entry->url, base_name, pool);
Given that parent_entry is checked earlier in the function I think
p_entry cannot be NULL. I don't why p_entry exists, but then this is
the very scary svn_wc_add2!
>
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 18538)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -716,7 +716,13 @@
> const svn_wc_entry_t *full_entry = entry;
>
> if (entry->kind == kind)
> - SVN_ERR(svn_wc_entry(&full_entry, path, adm_access, FALSE, pool));
> + {
> + SVN_ERR(svn_wc_entry(&full_entry, path, adm_access, FALSE, pool));
> + if (! full_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(path, pool));
> + }
>
> /* Descend only if the subdirectory is a working copy directory
> (and DESCEND is non-zero ofcourse) */
> @@ -799,6 +805,10 @@
>
> /* Get this directory's entry. */
> SVN_ERR(svn_wc_entry(&dir_entry, path, adm_access, FALSE, subpool));
> + if (! dir_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(path, pool));
path comes from svn_wc_adm_access_path so dir_entry cannot be NULL.
>
> /* Unless specified, add default ignore regular expressions and try
> to add any svn:ignore properties from the parent directory. */
> @@ -2054,8 +2064,15 @@
> SVN_ERR(svn_wc__adm_retrieve_internal(&parent_access, adm_access,
> parent_path, pool));
> if (parent_access)
> - SVN_ERR(svn_wc_entry(&parent_entry, parent_path, parent_access,
> - FALSE, pool));
> + {
> + SVN_ERR(svn_wc_entry(&parent_entry, parent_path, parent_access,
> + FALSE, pool));
> + /* XXX this check is questionable */
> + if (! parent_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(parent_path, pool));
parent_access is not NULL so parent_entry cannot be NULL.
> + }
> }
>
> SVN_ERR(assemble_status(status, path, adm_access, entry, parent_entry,
> Index: subversion/libsvn_wc/lock.c
> ===================================================================
> --- subversion/libsvn_wc/lock.c (revision 18538)
> +++ subversion/libsvn_wc/lock.c (working copy)
> @@ -1053,6 +1053,15 @@
> return err;
> }
>
> + if (! t_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(path, pool));
> + if (! p_entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(parent, pool));
> +
t_access is an access baton for path, so t_entry cannot be NULL.
p_access is an access baton for parent so p_entry cannot be NULL.
> /* Disjoint won't have PATH in P_ACCESS, switched will have
> incompatible URLs */
> if (! t_entry_in_p
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 18538)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -2058,7 +2058,12 @@
> /* Create strings representing the revisions of the
> old and new text-bases. */
> SVN_ERR(svn_wc_entry(&e, file_path, adm_access, FALSE, pool));
> - assert(e != NULL);
> + if (! e)
> + return svn_error_createf(
> + SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(file_path, pool));
> +
> oldrev_str = apr_psprintf(pool, ".r%ld",
> e->revision);
> newrev_str = apr_psprintf(pool, ".r%ld",
> @@ -2844,6 +2849,11 @@
> copyfrom URL to be in the same repository. */
> {
> SVN_ERR(svn_wc_entry(&ent, dir_name, adm_access, FALSE, pool));
> + if (! ent)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(dir_name, pool));
> +
> new_URL = svn_path_url_add_component(ent->url, base_name, pool);
>
> if (copyfrom_url && ent->repos &&
> Index: subversion/libsvn_wc/questions.c
> ===================================================================
> --- subversion/libsvn_wc/questions.c (revision 18538)
> +++ subversion/libsvn_wc/questions.c (working copy)
> @@ -269,8 +269,11 @@
> const svn_wc_entry_t *entry;
>
> SVN_ERR(svn_wc_entry(&entry, versioned_file, adm_access, TRUE, pool));
> + if (! entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(versioned_file, pool));
>
> -
> if (compare_textbases)
> SVN_ERR(svn_wc_translated_file2
> (&tmp_vfile, versioned_file,
> Index: subversion/libsvn_wc/translate.c
> ===================================================================
> --- subversion/libsvn_wc/translate.c (revision 18538)
> +++ subversion/libsvn_wc/translate.c (working copy)
> @@ -188,6 +188,10 @@
> }
>
> SVN_ERR(svn_wc_entry(&entry, path, adm_access, FALSE, pool));
> + if (! entry)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(path, pool));
>
> SVN_ERR(svn_subst_build_keywords2(keywords,
> list,
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org