Re: Relaxed STATUS voting rules for experimental features?

[Branko Čibej]

On 12.02.2018 16:00, Daniel Shahaf wrote:
> The shelving functionality is experimental:
>
> /** Shelve a change.
>  ⋮
>  * @since New in 1.10.
>  * @warning EXPERIMENTAL.
>  */
> SVN_EXPERIMENTAL
> svn_error_t *
> svn_client_shelve(const char *name,
>   const apr_array_header_t *paths,
>   svn_depth_t depth,
>   const apr_array_header_t *changelists,
>   svn_boolean_t keep_local,
>   svn_boolean_t dry_run,
>   svn_client_ctx_t *ctx,
>   apr_pool_t *pool);
>
> Shall we relax the STATUS voting criteria for such features?
>
> Currently they are "three +1's"; we could relax to "two +1's".
>
> WDYT?

Backporting experimental features isn't crucial, surely? Yet if there
are changes in trunk, they _might_ affect other code; in that case, more
eyes is better.

-- Brane

Re: [PATCH] Better error message for svn bindings import in mailer.py.

[Kenneth Porter]

On 2/12/2018 1:04 PM, Karl Fogel wrote:

* tools/hook-scripts/mailer/mailer.py: If the Subversion Python bindings
   could not be found for import at all, have the error message state
   that clearly, instead of duplicating the error message used for
   importing too old a version of the bindings.


I was also bitten by this when testing on a secondary system that lacked 
the bindings sub-package. I had to examine the source to understand what 
was really wrong. So +1.

[PATCH] Better error message for svn bindings import in mailer.py.

[Karl Fogel]

Hi all.  I think one of the import error messages in 
tools/hook-scripts/mailer/mailer.py is misleading (I discovered this while 
debugging the problem that Troy Curtis Jr. solved in r1823802).  Any objections 
if I install this patch?

[[[
Give correct error message on failure to import Subversion Python bindings.

* tools/hook-scripts/mailer/mailer.py: If the Subversion Python bindings
  could not be found for import at all, have the error message state
  that clearly, instead of duplicating the error message used for
  importing too old a version of the bindings.
]]]

Index: tools/hook-scripts/mailer/mailer.py
===
--- tools/hook-scripts/mailer/mailer.py (revision 1823993)
+++ tools/hook-scripts/mailer/mailer.py (working copy)
@@ -70,23 +70,21 @@
 _MIN_SVN_VERSION = [1, 5, 0]
 
 # Import the Subversion Python bindings, making sure they meet our
 # minimum version requirements.
 try:
   import svn.fs
   import svn.delta
   import svn.repos
   import svn.core
 except ImportError:
-  sys.stderr.write(
-"You need version %s or better of the Subversion Python bindings.\n" \
-% ".".join([str(x) for x in _MIN_SVN_VERSION]))
+  sys.stderr.write("Unable to import Subversion Python bindings.\n")
   sys.exit(1)
 if _MIN_SVN_VERSION > [svn.core.SVN_VER_MAJOR,
svn.core.SVN_VER_MINOR,
svn.core.SVN_VER_PATCH]:
   sys.stderr.write(
 "You need version %s or better of the Subversion Python bindings.\n" \
 % ".".join([str(x) for x in _MIN_SVN_VERSION]))
   sys.exit(1)
 
 

Re: svn commit: r1823966 - in /subversion/trunk/subversion/svn: shelf-cmd.c shelve-cmd.c

[Julian Foad]

Daniel Shahaf wrote:

julianf...@apache.org wrote on Mon, 12 Feb 2018 13:17 +:

+++ subversion/trunk/subversion/svn/shelf-cmd.c Mon Feb 12 13:17:16 2018
@@ -71,6 +71,36 @@ friendly_duration_str(apr_time_t duratio
+#ifndef WIN32
+/* Run CMD with ARGS.
+ * Send its stdout to the parent's stdout. Disconnect its stdin and stderr.
+ */
+static svn_error_t *
+run_cmd(const char *cmd,
+const char *const *args,
+apr_pool_t *scratch_pool)
+++ subversion/trunk/subversion/svn/shelve-cmd.c Mon Feb 12 13:17:16
2018
@@ -84,6 +84,36 @@ list_sorted_by_date(apr_array_header_t *
+#ifndef WIN32
+/* Run CMD with ARGS.
+ * Send its stdout to the parent's stdout. Disconnect its stdin and stderr.
+ */
+static svn_error_t *
+run_cmd(const char *cmd,
+const char *const *args,
+apr_pool_t *scratch_pool)


Why the duplication?  We could deduplicate by renaming to svn_cl__run_cmd()
and declaring it in subversion/svn/*.h.


The duplication is because 'shelve-cmd.c' is shelving-v1, left in trunk 
only for easier backporting to 1.10.x; while 'shelf-cmd.c' is the 
current shelving implementation.


I should rename them to make that clearer probably. Not sure what 
exactly. 'shelve-v1-cmd' and 'shelving-v2-cmd'? 'shelve-1.10.0-cmd.c'?


That was partly an experiment in feature-toggle development, although I 
didn't go far with it in this instance.



@@ -79,14 +109,19 @@ show_diffstat(svn_client_shelf_version_t
SVN_ERR(svn_client_shelf_get_patch_abspath(_abspath, shelf_version,
   scratch_pool));
+  args[0] = "diffstat";
+  args[1] = "-p0";
+  args[2] = patch_abspath;
+  args[3] = NULL;
+  err = run_cmd("diffstat", args, scratch_pool);
@@ -84,6 +84,36 @@ list_sorted_by_date(apr_array_header_t *
+  args[0] = "diffstat";
+  args[1] = "-p0";
+  args[2] = info->patch_path;
+  args[3] = NULL;


Maybe add "--" guards to these calls?  The path arguments are absolute, but
a "--" guard would minimize the impact of any quoting bug in svn_io_run_cmd().


Could do, and you are welcome to, but I think this is not important.

Thanks for reviewing and making these suggestions.

- Julian

Relaxed STATUS voting rules for experimental features?

[Daniel Shahaf]

The shelving functionality is experimental:

/** Shelve a change.
 ⋮
 * @since New in 1.10.
 * @warning EXPERIMENTAL.
 */
SVN_EXPERIMENTAL
svn_error_t *
svn_client_shelve(const char *name,
  const apr_array_header_t *paths,
  svn_depth_t depth,
  const apr_array_header_t *changelists,
  svn_boolean_t keep_local,
  svn_boolean_t dry_run,
  svn_client_ctx_t *ctx,
  apr_pool_t *pool);

Shall we relax the STATUS voting criteria for such features?

Currently they are "three +1's"; we could relax to "two +1's".

WDYT?

Cheers,

Daniel

Re: svn commit: r1823966 - in /subversion/trunk/subversion/svn: shelf-cmd.c shelve-cmd.c

[Daniel Shahaf]

julianf...@apache.org wrote on Mon, 12 Feb 2018 13:17 +:
> +++ subversion/trunk/subversion/svn/shelf-cmd.c Mon Feb 12 13:17:16 2018
> @@ -71,6 +71,36 @@ friendly_duration_str(apr_time_t duratio
> +#ifndef WIN32
> +/* Run CMD with ARGS.
> + * Send its stdout to the parent's stdout. Disconnect its stdin and stderr.
> + */
> +static svn_error_t *
> +run_cmd(const char *cmd,
> +const char *const *args,
> +apr_pool_t *scratch_pool)
> +++ subversion/trunk/subversion/svn/shelve-cmd.c Mon Feb 12 13:17:16 
> 2018
> @@ -84,6 +84,36 @@ list_sorted_by_date(apr_array_header_t *
> +#ifndef WIN32
> +/* Run CMD with ARGS.
> + * Send its stdout to the parent's stdout. Disconnect its stdin and stderr.
> + */
> +static svn_error_t *
> +run_cmd(const char *cmd,
> +const char *const *args,
> +apr_pool_t *scratch_pool)

Why the duplication?  We could deduplicate by renaming to svn_cl__run_cmd()
and declaring it in subversion/svn/*.h.

> @@ -79,14 +109,19 @@ show_diffstat(svn_client_shelf_version_t
>SVN_ERR(svn_client_shelf_get_patch_abspath(_abspath, shelf_version,
>   scratch_pool));
> +  args[0] = "diffstat";
> +  args[1] = "-p0";
> +  args[2] = patch_abspath;
> +  args[3] = NULL;
> +  err = run_cmd("diffstat", args, scratch_pool);
> @@ -84,6 +84,36 @@ list_sorted_by_date(apr_array_header_t *
> +  args[0] = "diffstat";
> +  args[1] = "-p0";
> +  args[2] = info->patch_path;
> +  args[3] = NULL;

Maybe add "--" guards to these calls?  The path arguments are absolute, but
a "--" guard would minimize the impact of any quoting bug in svn_io_run_cmd().

Cheers,

Daniel

Re: server crash in authz object pool

[Branko Čibej]

On 12.02.2018 14:27, Stefan Sperling wrote:
> On Mon, Feb 12, 2018 at 02:24:56PM +0100, Branko Čibej wrote:
>> Out of interest: why are you using APR without threads?
> To ensure that this configuration is still being tested.

That's good.

https://ci.apache.org/builders/svn-x64-macosx-apr1.3-nothread

One of our bots does that as well, but only with ra_local and ra_svn
because I only have the one httpd there, and it uses threaded APR.

-- Brane

Re: server crash in authz object pool

[Stefan Sperling]

On Mon, Feb 12, 2018 at 02:24:56PM +0100, Branko Čibej wrote:
> Out of interest: why are you using APR without threads?

To ensure that this configuration is still being tested.

Re: server crash in authz object pool

[Branko Čibej]

On 12.02.2018 13:07, Stefan Sperling wrote:
> On Mon, Feb 12, 2018 at 12:50:12PM +0100, Stefan Sperling wrote:
>> The svn-bb-openbsd bot has found a crash in the object pool code.
>>
>> It is triggered when running RA serf tests on OpenBSD. The httpd
>> work processes crash due to what looks like a use-after-free (0xdf
>> means this byte of memory has been freed).
>>
>> I can reproduce this on sparc64 and amd64 machines.
>> The build is without APR_HAS_THREADS, using the prefork MPM with HTTPD 2.2.
>> Let me know if you need more details. I have no time at present to hunt
>> this down myself, so I'm sending this info to the list in hope that it
>> will be useful.
> Hmm, a quick check revealed that this started happening with r1823787,
> where I stopped pre-loading libpthread.so into httpd. To the best of
> my knowledge dlopen() now works without this hack on OpenBSD.
>
> However, the httpd modules in my build *are* linked to libpthread.so
> for some reason, even though APR is compiled without threads:
>
> $ ldd modules/svn-trunk/{mod_authz_svn,mod_dav_svn,mod_dontdothat}.so | grep 
> pthread 
> 1afb97159000 1afb97362000 rlib 010   0  
> /usr/lib/libpthread.so.25.1
> 1afb2c499000 1afb2c6a2000 rlib 010   0  
> /usr/lib/libpthread.so.25.1
> 1afb81409000 1afb81612000 rlib 03   0  
> /usr/lib/libpthread.so.25.1
>
> So this could just be a local problem in my build.
> I'll investigate this later, and revert r1823787 for now.

Perhaps one of the other dependencies is pulling in pthreads?

Out of interest: why are you using APR without threads?

-- Brane

Re: shelves: system("diffstat -p0 %s")

[Julian Foad]

Philip Martin wrote:

Daniel Shahaf  writes:

Neither of these calls correctly quotes the path that's interpolated
into the command line.  Consequently, the wcroot_abspath would be
executed as shell commands.  Could that be fixed please?


Should probably do utf8 to local conversion as well.


Changed to use svn_io_run_cmd() which should fix both issues, in r1823966.

Thanks for the prod.
- Julian

Re: server crash in authz object pool

[Stefan Sperling]

On Mon, Feb 12, 2018 at 12:50:12PM +0100, Stefan Sperling wrote:
> The svn-bb-openbsd bot has found a crash in the object pool code.
> 
> It is triggered when running RA serf tests on OpenBSD. The httpd
> work processes crash due to what looks like a use-after-free (0xdf
> means this byte of memory has been freed).
> 
> I can reproduce this on sparc64 and amd64 machines.
> The build is without APR_HAS_THREADS, using the prefork MPM with HTTPD 2.2.
> Let me know if you need more details. I have no time at present to hunt
> this down myself, so I'm sending this info to the list in hope that it
> will be useful.

Hmm, a quick check revealed that this started happening with r1823787,
where I stopped pre-loading libpthread.so into httpd. To the best of
my knowledge dlopen() now works without this hack on OpenBSD.

However, the httpd modules in my build *are* linked to libpthread.so
for some reason, even though APR is compiled without threads:

$ ldd modules/svn-trunk/{mod_authz_svn,mod_dav_svn,mod_dontdothat}.so | grep 
pthread 
1afb97159000 1afb97362000 rlib 010   0  
/usr/lib/libpthread.so.25.1
1afb2c499000 1afb2c6a2000 rlib 010   0  
/usr/lib/libpthread.so.25.1
1afb81409000 1afb81612000 rlib 03   0  
/usr/lib/libpthread.so.25.1

So this could just be a local problem in my build.
I'll investigate this later, and revert r1823787 for now.


r1823787 | stsp | 2018-02-10 18:17:17 +0100 (Sat, 10 Feb 2018) | 3 lines

* tools/dev/unix-build/Makefile.svn: Remove the LIB_PTHREAD_HACK.
  This is no longer needed.


Index: tools/dev/unix-build/Makefile.svn
===
--- tools/dev/unix-build/Makefile.svn   (revision 1823786)
+++ tools/dev/unix-build/Makefile.svn   (revision 1823787)
@@ -1940,17 +1940,9 @@ endif
 libpath:
@echo export LD_LIBRARY_PATH="$(LD_LIBRARY_PATH):$$LD_LIBRARY_PATH" \
"PYTHONPATH=$(SVN_PREFIX)/lib/svn-python"
-#
-# OpenBSD requires an LD_PRELOAD hack to dlopen() libraries linked to
-# libpthread (e.g. libsvn_auth_gnome_keyring.so) into executables that
-# aren't linked to libpthread.
-ifeq ($(UNAME),OpenBSD)
-LIB_PTHREAD_HACK=LD_PRELOAD=libpthread.so
-endif
-
 .PHONY: start-svnserve stop-svnserve start-httpd stop-httpd
 
-HTTPD_CMD = env LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) $(LIB_PTHREAD_HACK) \
+HTTPD_CMD = env LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) \
$(PREFIX)/httpd/bin/apachectl
 HTTPD_START_CMD = $(HTTPD_CMD) -f $(HTTPD_CHECK_CONF) -k start
 HTTPD_START_CMD_PROXY = $(HTTPD_CMD) -f $(HTTPD_PROXY_CONF)
@@ -2007,7 +1999,7 @@ define do_check
 -cd $(svn_builddir) && for fs in fsfs bdb; do \
 echo "Begin test: $(subst svn-check-,,$@) x $$fs"; \
 test -d "$(RAMDISK)/tmp" && export TMPDIR="$(RAMDISK)/tmp"; \
-env LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) $(LIB_PTHREAD_HACK) \
+env LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) \
 env MAKEFLAGS= make check PARALLEL=$(PARALLEL) CLEANUP=$(CLEANUP) \
  EXCLUSIVE_WC_LOCKS=$(EXCLUSIVE_WC_LOCKS) \
  SVN_BIN_DIR=$(SVN_PREFIX)/bin \
@@ -2066,7 +2058,6 @@ svn-check-swig-pl:
-if [ $(ENABLE_PERL_BINDINGS) = yes ]; then \
(cd $(svn_builddir) && \
env LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) \
-   $(LIB_PTHREAD_HACK) \
env MAKEFLAGS= make check-swig-pl 2>&1) | \
tee $(svn_builddir)/tests.log.bindings.pl; \
fi
@@ -2084,8 +2075,7 @@ svn-check-swig-rb:
env RUBYLIB=$(RUBYLIB) \
LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) \
PATH=$(SVN_PREFIX)/bin:$$PATH \
-   $(LIB_PTHREAD_HACK) \
-   env MAKEFLAGS= make check-swig-rb 2>&1) | \
+   MAKEFLAGS= make check-swig-rb 2>&1) | \
tee $(svn_builddir)/tests.log.bindings.rb
 
 svn-check-javahl:

server crash in authz object pool

[Stefan Sperling]

The svn-bb-openbsd bot has found a crash in the object pool code.

It is triggered when running RA serf tests on OpenBSD. The httpd
work processes crash due to what looks like a use-after-free (0xdf
means this byte of memory has been freed).

I can reproduce this on sparc64 and amd64 machines.
The build is without APR_HAS_THREADS, using the prefork MPM with HTTPD 2.2.
Let me know if you need more details. I have no time at present to hunt
this down myself, so I'm sending this info to the list in hope that it
will be useful.

#2  0x0efde1f5550d in lookup (object=0xefe4c595ac0, 
object_pool=0xefe97bc4f60, key=0xefe97bc4ac0, result_pool=0xefddbb18500)
at subversion/libsvn_subr/object_pool.c:193
193 = apr_hash_get(object_pool->objects, key->data, key->size);
(gdb) p *object_pool->objects
$3 = {pool = 0xdfdfdfdfdfdfdfdf, array = 0xdfdfdfdfdfdfdfdf, iterator = {ht = 
0x0, this = 0x0, next = 0xdfdfdfdfdfdfdfdf, index = 3755991007},
  count = 3755991007, max = 3755991007, seed = 3755991007, hash_func = 
0xdfdfdfdfdfdfdfdf, free = 0xdfdfdfdfdfdfdfdf}
(gdb) bt
#0  0x0efe4199790c in find_entry (ht=0x2e646e762f6e6f69, key=0xefec8a8aa40, 
klen=16, val=0x0)
at /home/stsp/svn/src/apr-1.5.2/tables/apr_hash.c:293
#1  0x0efe41997c7a in apr_hash_get (ht=0x2e646e762f6e6f69, 
key=0xefec8a8aa40, klen=16) at 
/home/stsp/svn/src/apr-1.5.2/tables/apr_hash.c:367
#2  0x0efde1f5550d in lookup (object=0xefe4c595ac0, 
object_pool=0xefe97bc4f60, key=0xefe97bc4ac0, result_pool=0xefddbb18500)
at subversion/libsvn_subr/object_pool.c:193
#3  0x0efde1f55814 in svn_object_pool__lookup (object=0xefe4c595ac0, 
object_pool=0xefe97bc4f60, key=0xefe97bc4ac0,
result_pool=0xefddbb18500) at subversion/libsvn_subr/object_pool.c:309
#4  0x0efe40b70459 in authz_read (authz_p=0xefe4c595ac0, 
authz_id=0xefe4c595ac8,
---Type  to continue, or q  to quit---
path=0xefe5f813680 
"/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svn-test-work/authz", 
groups_path=0x0, must_exist=1, repos_hint=0x0,
result_pool=0xefddbb18500, scratch_pool=0xefddbb1a600) at 
subversion/libsvn_repos/authz.c:1579
#5  0x0efe40b7067f in svn_repos_authz_read3 (authz_p=0x7f7e2170,
path=0xefe5f813680 
"/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svn-test-work/authz", 
groups_path=0x0, must_exist=1, repos_hint=0x0,
result_pool=0xefddbb18500, scratch_pool=0xefddbb1a600) at 
subversion/libsvn_repos/authz.c:1642
#6  0x0efecc0ae158 in get_access_conf (r=0xefe412bb400, conf=0xefe1d05af00, 
scratch_pool=0xefddbb1a600)
at subversion/mod_authz_svn/mod_authz_svn.c:467
#7  0x0efecc0ae7c6 in req_check_access (r=0xefe412bb400, 
conf=0xefe1d05af00, repos_path_ref=0x7f7e2320,
---Type  to continue, or q  to quit---
dest_repos_path_ref=0x7f7e2318) at 
subversion/mod_authz_svn/mod_authz_svn.c:690
#8  0x0efecc0aefb0 in auth_checker (r=0xefe412bb400) at 
subversion/mod_authz_svn/mod_authz_svn.c:1036
#9  0x0efbd47194bb in ap_run_auth_checker (r=0xefe412bb400) at 
/home/stsp/svn/src/httpd-2.2.32/server/request.c:78
#10 0x0efbd4719b87 in ap_process_request_internal (r=0xefe412bb400) at 
/home/stsp/svn/src/httpd-2.2.32/server/request.c:198
#11 0x0efbd478241d in ap_process_request (r=0xefe412bb400) at 
/home/stsp/svn/src/httpd-2.2.32/modules/http/http_request.c:296
#12 0x0efbd477f051 in ap_process_http_connection (c=0xefde19a0200) at 
/home/stsp/svn/src/httpd-2.2.32/modules/http/http_core.c:190
#13 0x0efbd4729a83 in ap_run_process_connection (c=0xefde19a0200) at 
/home/stsp/svn/src/httpd-2.2.32/server/connection.c:43
#14 0x0efbd4729f2d in ap_process_connection (c=0xefde19a0200, 
csd=0xefde5a3b300) at /home/stsp/svn/src/httpd-2.2.32/server/connection.c:190
---Type  to continue, or q  to quit---
#15 0x0efbd47b5d6e in child_main (child_num_arg=5) at 
/home/stsp/svn/src/httpd-2.2.32/server/mpm/prefork/prefork.c:667
#16 0x0efbd47b5f4f in make_child (s=0xefe150eda00, slot=5) at 
/home/stsp/svn/src/httpd-2.2.32/server/mpm/prefork/prefork.c:768
#17 0x0efbd47b61e9 in perform_idle_server_maintenance (p=0xefe150ed500) at 
/home/stsp/svn/src/httpd-2.2.32/server/mpm/prefork/prefork.c:903
#18 0x0efbd47b674b in ap_mpm_run (_pconf=0xefe150ed500, plog=0xefe150ed800, 
s=0xefe150eda00)
at /home/stsp/svn/src/httpd-2.2.32/server/mpm/prefork/prefork.c:1107
#19 0x0efbd4703e23 in main (argc=5, argv=0x7f7e2878) at 
/home/stsp/svn/src/httpd-2.2.32/server/main.c:753
(gdb)