[PATCH] subversion issue #4174 - serf: checkout / export over https errors out with svn: E730053: Error retrieving REPORT (730053)
Hi Johan,
as you seem to be the only one encountering issue 4174, would you mind
testing attached serf patch?
You'll have to update serf trunk to r1627 to apply it cleanly.
It fixes the issue for me on Windows 7 with svn trunk and serf trunk.
While I have tested the patches on Mac OS X too, I couldn't reproduce
the original problem.
[[[
Fix https onnection abort issue when using https. This should fix
Subversion issue 4174:
http://subversion.tigris.org/issues/show_bug.cgi?id=4174
When the server sends us a close notify SSL Alert indicating that it's
going to close the socket, we have to stop sending on that socket immediately.
Failing to do so will make the server reset the connection, which can result
in an ECONNABORTED error upon next read or write. This seems to happen esp.
on Windows.
* buckets/ssl_buckets.c
(ssl_decrypt): when SSL_read returns ok but with length 0, a close notify
alert was received. Return APR_EOF to let upstream know that we're done
with this socket.
* outgoing.c
(read_from_connection): Add comment to note that SSL alert's are also an
example of unexpected incoming data.
]]]
regards,
Lieven
Index: outgoing.c
===
--- outgoing.c (revision 1627)
+++ outgoing.c (working copy)
@@ -945,10 +945,12 @@ static apr_status_t read_from_connection(serf_conn
* 2) Doing the initial SSL handshake - we'll get EAGAIN
*as the SSL buckets will hide the handshake from us
*but not return any data.
+ * 3) When the server sends us an SSL alert.
*
* In these cases, we should not receive any actual user data.
*
- * If we see an EOF (due to an expired timeout), we'll reset the
+ * If we see an EOF (due to either an expired timeout or the serer
+ * sending the SSL 'close notify' shutdown alert), we'll reset the
* connection and open a new one.
*/
if (request-req_bkt || !request-written) {
Index: buckets/ssl_buckets.c
===
--- buckets/ssl_buckets.c (revision 1627)
+++ buckets/ssl_buckets.c (working copy)
@@ -626,7 +626,15 @@ static apr_status_t ssl_decrypt(void *baton, apr_s
break;
}
}
-else {
+else if (ssl_len == 0 status == 0) {
+ /* The server shut down the connection. */
+ *len = 0;
+#ifdef SSL_VERBOSE
+ printf(ssl_decrypt: SSL read error: server shut down\
+connection!\n);
+#endif
+status = APR_EOF;
+} else {
*len = ssl_len;
#ifdef SSL_VERBOSE
printf(---\n%.*s\n-(%d)-\n, *len, buf, *len);
Re: [Issue 4124] New - Give servers sufficient means to disallow commits from clients based on version numbers
C. Michael Pilato wrote on Fri, Aug 17, 2012 at 14:01:03 -0400: On 08/17/2012 12:43 PM, Daniel Shahaf wrote: Stefan Sperling wrote on Fri, Aug 17, 2012 at 18:24:03 +0200: On Fri, Aug 17, 2012 at 12:14:31PM -0400, C. Michael Pilato wrote: 4. something else? 4. Marshall the version string before sending it across svn://, escaping unsupported characters somehow in a reversible way, and unescape them before passing them to hooks? I.e. use something like strnvis() but adapted to the restrictions of the svn:// protocol. Yeah, I forgot to add that I was thinking about this approach, too. I'm not familiar with strnvis(), but I assume it's similar to what we do with our checksum API _readable() variants (where 'A' is 0x41 and represented as as I don't see any public or interlibrary API with /_readable/ in its name?
Re: [Issue 4124] New - Give servers sufficient means to disallow commits from clients based on version numbers
Branko Čibej wrote on Fri, Aug 17, 2012 at 20:07:34 +0200: On 17.08.2012 20:01, C. Michael Pilato wrote: On 08/17/2012 12:43 PM, Daniel Shahaf wrote: Stefan Sperling wrote on Fri, Aug 17, 2012 at 18:24:03 +0200: On Fri, Aug 17, 2012 at 12:14:31PM -0400, C. Michael Pilato wrote: 4. something else? 4. Marshall the version string before sending it across svn://, escaping unsupported characters somehow in a reversible way, and unescape them before passing them to hooks? I.e. use something like strnvis() but adapted to the restrictions of the svn:// protocol. Yeah, I forgot to add that I was thinking about this approach, too. I'm not familiar with strnvis(), but I assume it's similar to what we do with our checksum API _readable() variants (where 'A' is 0x41 and represented as as 41)? 5. Require the version string and client name to match /^[A-Za-z0-9-]+$/, and embed them directly as part of a capability name (so: capabilities might be named client-version-tsvn-1dot7dot0) I'll give the honor of treating this suggestion as a serious one. And then the dishonor of a hearty -1. :-) Why? It's essentially the same as 4., just with a different and higher-level marshalling. For the record, I don't really object to the -1... the suggestion violates layering and hardcodes restriction where they don't quite belong. Its advantage is --- like stsp's suggestion --- not requiring a protocol version bump. And then there is ghudson's point, of course.
Re: Please move mod_dontdothat back from tools/server-side/mod_dontdothat to subversion/mod_dontdothat
On 18.08.2012 16:53, Nico Kadel-Garcia wrote: There was a thread on this in us...@subvesion.org, I think I've boiled it down enough for dev to look at it. To sum up, please revert r1307177, and move tools/server-side/mod_dontdothat back to the subversion/mod_dontdothat. Unless there is a plan to migrate *all* the subversion/mod_* components over to tools/server-side/mod_*, I'm looking at the code and just don't see why this component should should be so distinct. I'm afraid that sprinkling the HTTPD modules into separate compilation and deployment locations is just going to create ongoing surprises. It already bit me in RPM compilation and management. I see the code danielsh wen into to do this, but unless the mod_dav_svn and mod_authz_svn tools are also going to migrate to tools/server-side, I think it's going to be safer to manage whee it was in the subversion-1.7.5 tag. Nico Kadel-Garcia nka...@gmail.com mod_dontdothat is not a core module. It's effectively a debugging tool. Whereas mod_dav_svn and mod_authz_svn are part of the core Subversion functionality. The different locations aren't accidental. The main problem with moving mod_dontdothat was that the way it was done broke Unix builds without Apache HTTPd. That's fixed on trunk now, and could probably be backported to 1.7.x. -- Brane -- Certified Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: Please move mod_dontdothat back from tools/server-side/mod_dontdothat to subversion/mod_dontdothat
On 08/19/2012 11:02 AM, Branko Čibej wrote: On 18.08.2012 16:53, Nico Kadel-Garcia wrote: mod_dontdothat is not a core module. It's effectively a debugging tool. Whereas mod_dav_svn and mod_authz_svn are part of the core Subversion functionality. The different locations aren't accidental. The main problem with moving mod_dontdothat was that the way it was done broke Unix builds without Apache HTTPd. That's fixed on trunk now, and could probably be backported to 1.7.x. There's some downstream maintainers I know that would appreciate that ;) Which revisions need backporting so I can review and vote on them? Blair
