You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@subversion.apache.org by Joe Orton <jo...@manyfish.co.uk> on 2002/12/20 22:04:33 UTC

Re: [neon] [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

On Fri, Dec 20, 2002 at 06:19:01AM -0800, Kevin Pilch-Bisson wrote:
> While running svn merge URL URL, we overrun a 4K buffer size max in neon 
> and this caused subvesion to exit with an error before finishing its 
> work.  The buffer in question is in new_socket.c line 164 "#define 
> RDBUFSIZ 4096".  After upping this value by multiplying it by 32 the 
> operation finished successfully.

Can you explain why this is a problem, what causes this failure (with a
debug trace)? neon should work correctly (albeit not optimally) with any
size of read buffer.

Regards,

joe

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Mon, Dec 23, 2002 at 06:59:44PM +0100, Johan Lindh wrote:
> >Thanks Johan. I took that, cleaned it up a bit, and extended it to map
> >the return values properly.  Does the attached patch still work?
> 
> Um, without actually testing it... from reading the code, it seems like
> set_strerror() will not now return an error message if the return value
> from WSAGetLastError() is unknown. 

That's right, set_strerror doesn't return anything.

> More importantly, if ne_snprintf() takes a char * as it's first argument,
> then you're bashing the ne_socket structure with that call.
> 
> Perhaps ne_snprintf( s->error, ..... ) was intended?

Good point - thanks.  Updated patch attached.

joe

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Johan Lindh <jo...@linkdata.se>.

>Thanks Johan. I took that, cleaned it up a bit, and extended it to map
>the return values properly.  Does the attached patch still work?

Um, without actually testing it... from reading the code, it seems like
set_strerror() will not now return an error message if the return value
from WSAGetLastError() is unknown. 

More importantly, if ne_snprintf() takes a char * as it's first argument,
then you're bashing the ne_socket structure with that call.

Perhaps ne_snprintf( s->error, ..... ) was intended?


/J



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Mon, Dec 23, 2002 at 03:39:10PM +0100, Johan Lindh wrote:
> On 2002-12-23 at 00:39 Joe Orton wrote:
> >The code in question is all inside neon's socket abstraction,
> >ne_socket.[ch] - only read_raw/write_raw/readable_raw should need
> >changing, it should be pretty clear what needs to be done.
> 
> Unfortunately, ./configure from neon cvs bombs on my machine, so I couldn't test this properly.
> But anyway, here's a diff that at least does the minimum. That is, changes set_strerror() to
> a) write the Win32 error message and b) change the value of errno to something more reasonable.
> 
> As is, the patch will not affect the return values in a consistent way, though, just the error message.
> If the read_raw() and others would call set_strerror() -before- checking the value of errno, the
> return value should get meaningful values on Win32 too.

Thanks Johan. I took that, cleaned it up a bit, and extended it to map
the return values properly.  Does the attached patch still work?

Brandon, can you apply the attached patch (and undo the buffer size
change) then recompile neon/SVN and try your merge failure case? What
error message do you get now?

Regards,

joe

Re: [neon] Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Johan Lindh <jo...@linkdata.se>.

On 2002-12-23 at 00:39 Joe Orton wrote:
>The code in question is all inside neon's socket abstraction,
>ne_socket.[ch] - only read_raw/write_raw/readable_raw should need
>changing, it should be pretty clear what needs to be done. 

Unfortunately, ./configure from neon cvs bombs on my machine, so I couldn't test this properly.
But anyway, here's a diff that at least does the minimum. That is, changes set_strerror() to
a) write the Win32 error message and b) change the value of errno to something more reasonable.

As is, the patch will not affect the return values in a consistent way, though, just the error message.
If the read_raw() and others would call set_strerror() -before- checking the value of errno, the
return value should get meaningful values on Win32 too.

/J

--- ne_socket.c.org	Mon Dec 23 15:18:01 2002
+++ ne_socket.c	Mon Dec 23 15:26:02 2002
@@ -52,6 +52,23 @@
 #ifdef WIN32
 #include <winsock2.h>
 #include <stddef.h>
+#include <errno.h>
+#if !defined(ECONNRESET)
+#define ECONNRESET WSAECONNRESET
+#endif
+#if !defined(EINTR)
+#define EINTR WSAEINTR
+#endif
+#if !defined(EFAULT)
+#define EFAULT WSAEFAULT
+#endif
+#if !defined(ENETDOWN)
+#define ENETDOWN WSAENETDOWN
+#endif
+#if !defined(ENOTCONN)
+#define ENOTCONN WSAENOTCONN
+#endif
+static const char* ne_win32_map_errno(void);
 #else
 #include <netinet/in.h>
 #include <netdb.h>
@@ -181,7 +198,39 @@
 #define set_error(s, str) ne_strnzcpy((s)->error, (str), sizeof (s)->error)
 
 /* set_strerror: set socket error to system error string for 'e' */
+#ifdef WIN32
+static const char* ne_win32_map_errno(void)
+{
+    switch( WSAGetLastError() )
+    {
+       case WSANOTINITIALISED: errno=ENOENT; return "A successful WSAStartup call must occur before using this function.";
+       case WSAENETDOWN: errno=ENETDOWN; return "The network subsystem has failed.";
+       case WSAEFAULT: errno=EFAULT; return "Referenced data outside the process address space";
+       case WSAENOTCONN: errno=ENOTCONN; return "The socket is not connected.";
+       case WSAEINTR: errno=EINTR; return "Call was interrupted.";
+       case WSAEINPROGRESS: errno=EINTR; return "A blocking call is in progess.";
+       case WSAENETRESET: errno=ECONNRESET; return "The connection was reset during keep-alive activity.";
+       case WSAENOTSOCK: errno=EBADF; return "The descriptor is not a socket.";
+       case WSAEOPNOTSUPP: errno=EINVAL; return "Operation not supported for this socket type.";
+       case WSAESHUTDOWN: errno=ENOTCONN; return "The socket has been shut down.";
+       case WSAEWOULDBLOCK: errno=EAGAIN; return "The socket is marked as nonblocking and the receive operation would block.";
+       case WSAEMSGSIZE: errno=EINVAL; return "The message was too large to fit into the specified buffer and was truncated.";
+       case WSAEINVAL: errno=EINVAL; return "The socket has not been bound with bind, or an unknown/unsupported flag/operation was specified.";
+       case WSAECONNABORTED: errno=ECONNRESET; return "The virtual circuit was terminated due to a time-out or other failure.";
+       case WSAETIMEDOUT: errno=EAGAIN; return "The connection has been dropped because of a network failure or because the peer system failed to respond.";
+       case WSAECONNRESET: errno=ECONNRESET; return "The virtual circuit was reset by the remote side executing a hard or abortive close.";
+       default: break;
+    }
+    return strerror(errno);
+}
+static void set_strerror(ne_socket *s, int e)
+{
+    strncpy( s->error, ne_win32_map_errno(), sizeof( s->error ) );
+    s->error[ sizeof( s->error ) - 1 ] = 0;
+}
+#else
 #define set_strerror(s, e) ne_strerror((e), (s)->error, sizeof (s)->error)
+#endif
 
 #ifdef NEON_SSL
 static int prng_seeded = 0; 





---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Sun, Dec 22, 2002 at 10:43:02PM +0100, Branko Čibej wrote:
> Joe Orton wrote:
> >On Sun, Dec 22, 2002 at 06:34:15PM +0100, Branko Čibej wrote:
> >>Judjung by the Windows Socket API docs, /nont/ of the socket functions
> >>use errno on Windows. There's a (non-standard, of course) function that
> >>returns the latest error code. Do you think this would work?
> >>
> >>#ifdef WIN32
> >>#define NEON_SOCK_ERRNO() errno
> >>#else
> >>#define NEON_SOCK_ERRNO() WSAGetLastError()
> >>#endif
> >>
> >>Then use that macro wherever errno is being checked after a socket call.
> >
> >OK - a few questions: can the return value from WSAGetLastError() be
> >passed to strerror() to get useful error strings,
> >
> Hmm, don't know. I can test it, though. Let's see... Nope, it's in a
> different error space, which implies...
> 
> > and compared with
> >ECONNRESET to detect a connection reset?
> >
> ...that you can't do that, etither.
> 
> >Is select() one of the functions which doesn't use errno on Win32?
> >
> Oh, yes. It looks like none of them does.
> 
> Do I suspect correctly that Neon needs a bit of abstraction for sockets,
> at least for error handling?

The code in question is all inside neon's socket abstraction,
ne_socket.[ch] - only read_raw/write_raw/readable_raw should need
changing, it should be pretty clear what needs to be done. 

e.g. for read_raw, if the recv() fails, return NE_SOCK_RESET if the
connection has been reset, or NE_SOCK_ERROR otherwise; either way, copy
an error string into the sock->error buffer.

Regards,

joe

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Branko Čibej <br...@xbc.nu>.

Joe Orton wrote:

>On Sun, Dec 22, 2002 at 06:34:15PM +0100, Branko Čibej wrote:
>
>>Judjung by the Windows Socket API docs, /nont/ of the socket functions
>>use errno on Windows. There's a (non-standard, of course) function that
>>returns the latest error code. Do you think this would work?
>>
>>#ifdef WIN32
>>#define NEON_SOCK_ERRNO() errno
>>#else
>>#define NEON_SOCK_ERRNO() WSAGetLastError()
>>#endif
>>
>>Then use that macro wherever errno is being checked after a socket call.
>>    
>>
>
>OK - a few questions: can the return value from WSAGetLastError() be
>passed to strerror() to get useful error strings,
>
Hmm, don't know. I can test it, though. Let's see... Nope, it's in a
different error space, which implies...

> and compared with
>ECONNRESET to detect a connection reset?
>
...that you can't do that, etither.

>Is select() one of the functions which doesn't use errno on Win32?
>
Oh, yes. It looks like none of them does.

Do I suspect correctly that Neon needs a bit of abstraction for sockets,
at least for error handling?

-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Sun, Dec 22, 2002 at 11:46:51AM -0500, Brandon Ehle wrote:
> >
> >
> >>>Thanks Ben - it looks like the client is on Win32 here? It's a known bug
> >>>in neon that recv() errors aren't handled properly on Win32 - this is
> >>>hiding the real error message with
> >>>
> >>>"Could not read response body: No error"
> >>>
> I don't think any error has occured at this point, so the "No error" 
> text should be perfectly correct.  There is a bug in neon that's 
> preventing Subversion from recieving the rest of the stream.  By upping 
> the read buffer from 4096 to something higher you can around this bug.

It looks clear to me that the debug trace Ben posted show ne_sock_read
returning an error; yes, increasing the read buffer size may mean you
can no longer trigger this error.

Regards,

joe

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Brandon Ehle <be...@pipedreaminteractive.com>.

> 
>
>>>Thanks Ben - it looks like the client is on Win32 here? It's a known bug
>>>in neon that recv() errors aren't handled properly on Win32 - this is
>>>hiding the real error message with
>>>
>>> "Could not read response body: No error"
>>>
I don't think any error has occured at this point, so the "No error" 
text should be perfectly correct.  There is a bug in neon that's 
preventing Subversion from recieving the rest of the stream.  By upping 
the read buffer from 4096 to something higher you can around this bug.



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Sun, Dec 22, 2002 at 06:34:15PM +0100, Branko Čibej wrote:
> Joe Orton wrote:
...
> >Thanks Ben - it looks like the client is on Win32 here? It's a known bug
> >in neon that recv() errors aren't handled properly on Win32 - this is
> >hiding the real error message with
> >
> >  "Could not read response body: No error"
> >
> >so I can't tell what's really gone wrong; this needs a Win32-savvy
> >person to go and fix. (On Win32 it seems that recv() doesn't use errno;
> >ne_socket.c:read_raw() and write_raw() both need changing for this I
> >suspect)
> >  
> >
> 
> Judjung by the Windows Socket API docs, /nont/ of the socket functions
> use errno on Windows. There's a (non-standard, of course) function that
> returns the latest error code. Do you think this would work?
> 
> #ifdef WIN32
> #define NEON_SOCK_ERRNO() errno
> #else
> #define NEON_SOCK_ERRNO() WSAGetLastError()
> #endif
> 
> Then use that macro wherever errno is being checked after a socket call.

OK - a few questions: can the return value from WSAGetLastError() be
passed to strerror() to get useful error strings, and compared with
ECONNRESET to detect a connection reset? Is select() one of the
functions which doesn't use errno on Win32?

> Oh, BTW: There's a symmetric function, WSASetLastError(), that can be
> used to change the code that WSAGetLastError returns. I don't know if
> you need that (i.e., if you do eny errno assignments within Neon), but
> you can't cal it without first calling (yet another) Windows-specific
> socket lib initialization function..

There is one errno assignment in ne_socket.c but it appears to be
redundant, so it doesn't look like that will be needed.

Thanks!

joe

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Branko Čibej <br...@xbc.nu>.

Branko Čibej wrote:

>Judjung by the Windows Socket API docs, /nont/
>
...

I think I'll stop working on Subversion, and start writing a and
on-the-fly spelling checking keyboard driver...

-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Branko Čibej <br...@xbc.nu>.

Joe Orton wrote:

>On Fri, Dec 20, 2002 at 05:00:07PM -0600, Ben Collins-Sussman wrote:
>  
>
>>Joe Orton <jo...@manyfish.co.uk> writes:
>>
>>    
>>
>>>The other time readline is used is for reading response header fields:
>>>I'd question whether it's reasonable for a response header field to be
>>>longer than 4096 chars too.  Is mod_dav_svn really sending these in some
>>>case? Please send network/debug traces!
>>>      
>>>
>>Joe, here's what you need:
>>
>>   http://newton.ch.collab.net/~sussman/report_dialogue.txt
>>
>>   http://newton.ch.collab.net/~sussman/neon_log.txt
>>    
>>
>
>Thanks Ben - it looks like the client is on Win32 here? It's a known bug
>in neon that recv() errors aren't handled properly on Win32 - this is
>hiding the real error message with
>
>  "Could not read response body: No error"
>
>so I can't tell what's really gone wrong; this needs a Win32-savvy
>person to go and fix. (On Win32 it seems that recv() doesn't use errno;
>ne_socket.c:read_raw() and write_raw() both need changing for this I
>suspect)
>  
>

Judjung by the Windows Socket API docs, /nont/ of the socket functions
use errno on Windows. There's a (non-standard, of course) function that
returns the latest error code. Do you think this would work?

#ifdef WIN32
#define NEON_SOCK_ERRNO() errno
#else
#define NEON_SOCK_ERRNO() WSAGetLastError()
#endif

Then use that macro wherever errno is being checked after a socket call.

Oh, BTW: There's a symmetric function, WSASetLastError(), that can be
used to change the code that WSAGetLastError returns. I don't know if
you need that (i.e., if you do eny errno assignments within Neon), but
you can't cal it without first calling (yet another) Windows-specific
socket lib initialization function..

-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [azverkan@yahoo.com: Joe Orton please read: Overrrun a neon buffer size during svn merge URL URL]

Posted by Joe Orton <jo...@manyfish.co.uk>.

On Fri, Dec 20, 2002 at 05:00:07PM -0600, Ben Collins-Sussman wrote:
> Joe Orton <jo...@manyfish.co.uk> writes:
> 
> > The other time readline is used is for reading response header fields:
> > I'd question whether it's reasonable for a response header field to be
> > longer than 4096 chars too.  Is mod_dav_svn really sending these in some
> > case? Please send network/debug traces!
> 
> Joe, here's what you need:
> 
>    http://newton.ch.collab.net/~sussman/report_dialogue.txt
>
>    http://newton.ch.collab.net/~sussman/neon_log.txt

Thanks Ben - it looks like the client is on Win32 here? It's a known bug
in neon that recv() errors aren't handled properly on Win32 - this is
hiding the real error message with

  "Could not read response body: No error"

so I can't tell what's really gone wrong; this needs a Win32-savvy
person to go and fix. (On Win32 it seems that recv() doesn't use errno;
ne_socket.c:read_raw() and write_raw() both need changing for this I
suspect)

Regards,

joe

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org