[PATCH] adm_files.c:adm_subdir()

[Joe Orton <jo...@light.plus.com>]

Is this function safe? As I read it, it allocates a string in a pool,
but expects it to last for the lifetime of the process.

Could do something complex and not very efficient with
apr_set/get_userdata, but maybe something simpler is okay for the time
being:

--- adm_files.c.orig	Sat Sep  9 08:51:56 2000
+++ adm_files.c	Mon Sep 11 11:01:35 2000
@@ -73,15 +73,10 @@
 /* No one outside this file should ever need to know this.  In fact,
    no one outside adm_subdir() should ever need to know this. */
 #define SVN_WC__ADM_DIR_DEFAULT   "SVN"
-static svn_string_t *
+static const char *
 adm_subdir (apr_pool_t *pool)
 {
-  static svn_string_t *adm_dir_str = NULL;
-
-  if (! adm_dir_str)
-    adm_dir_str = svn_string_create (SVN_WC__ADM_DIR_DEFAULT, pool);
-
-  return adm_dir_str;
+  return SVN_WC__ADM_DIR_DEFAULT;
 }
 
 
@@ -106,7 +101,8 @@
 {
   int components_added = 0;
 
-  svn_path_add_component (path, adm_subdir (pool), SVN_PATH_LOCAL_STYLE, pool);
+  svn_path_add_component_nts (path, adm_subdir (pool), SVN_PATH_LOCAL_STYLE, 
+                              pool);
   components_added++;
 
   if (adm_file && (adm_file[0] != '\0'))

Re: [PATCH] adm_files.c:adm_subdir()

[Karl Fogel <kf...@galois.collab.net>]

Greg Stein <gs...@lyra.org> writes:
> I agree. I've applied your patch (plus a bit). Karl may have different ideas
> on what to do here, but he can add more tweaks as needed.

Oh, great, thanks Greg.  I'll update; I doubt I'll have any changes to
make, but will tweak if needed.

Re: [PATCH] adm_files.c:adm_subdir()

[Greg Stein <gs...@lyra.org>]

On Mon, Sep 11, 2000 at 11:21:21AM +0100, Joe Orton wrote:
> Is this function safe?

Nope.

I just did a search over the libraries and found a second non-const global
variable (svn_delta__tagmap). That was used as "const", so I just checked in
a change to truly make it that way.

Basic rule: global mutable variables in SVN, anywhere and for whatever
purpose, are always a bad sign and invariably need to change.

> As I read it, it allocates a string in a pool,
> but expects it to last for the lifetime of the process.
> 
> Could do something complex and not very efficient with
> apr_set/get_userdata, but maybe something simpler is okay for the time
> being:

I agree. I've applied your patch (plus a bit). Karl may have different ideas
on what to do here, but he can add more tweaks as needed.

thx!
-g

-- 
Greg Stein, http://www.lyra.org/

Re: [PATCH] adm_files.c:adm_subdir()

[Karl Fogel <kf...@galois.collab.net>]

Joe Orton <jo...@light.plus.com> writes:
> Is this function safe? As I read it, it allocates a string in a pool,
> but expects it to last for the lifetime of the process.

Seems unsafe to me too, thanks for noticing.  I needed a "guardian"
function right away for accessing the adm area, so no other part of
the code would ever be tempted to hardcode the "./SVN/" path, so
that's what this is.  At least it's unsafe in exactly one place. :-)

I'll fix it.

> Could do something complex and not very efficient with
> apr_set/get_userdata, but maybe something simpler is okay for the time
> being:
> 
> --- adm_files.c.orig	Sat Sep  9 08:51:56 2000
> +++ adm_files.c	Mon Sep 11 11:01:35 2000
> @@ -73,15 +73,10 @@
>  /* No one outside this file should ever need to know this.  In fact,
>     no one outside adm_subdir() should ever need to know this. */
>  #define SVN_WC__ADM_DIR_DEFAULT   "SVN"
> -static svn_string_t *
> +static const char *
>  adm_subdir (apr_pool_t *pool)
>  {
> -  static svn_string_t *adm_dir_str = NULL;
> -
> -  if (! adm_dir_str)
> -    adm_dir_str = svn_string_create (SVN_WC__ADM_DIR_DEFAULT, pool);
> -
> -  return adm_dir_str;
> +  return SVN_WC__ADM_DIR_DEFAULT;
>  }
>  
>  
> @@ -106,7 +101,8 @@
>  {
>    int components_added = 0;
>  
> -  svn_path_add_component (path, adm_subdir (pool), SVN_PATH_LOCAL_STYLE, pool);
> +  svn_path_add_component_nts (path, adm_subdir (pool), SVN_PATH_LOCAL_STYLE, 
> +                              pool);
>    components_added++;
>  
>    if (adm_file && (adm_file[0] != '\0'))