Re: [Lynx-dev] SOCK5 crash+fix
Thomas Dickey wrote in <20220312191941.ga15...@prl-debianold-64.jexium-island.net>: |On Thu, Mar 10, 2022 at 07:18:46PM -0500, Thomas Dickey wrote: |> On Thu, Mar 10, 2022 at 07:22:21PM +0100, Steffen Nurpmeso wrote: |>> Hello. |>> |>> I was the original author of the $SOCKS5_PROXY patch, but for one |>> Thomas Dickey changed the C enormously, and i also have forgotten |>> anything about it, but you noting it made me look. |> |> sorry - I didn't notice this at the time, but it was in Steffen's patch: | |https://github.com/ThomasDickey/lynx-snapshots/commit/3f9967cb255c102f7c\ |0b09536fb6665392b013c6 Yes, thanks for the NETWRITE() changes. That HTSprintf0() hunk however is quite a no-op, it wastes a precious return register :). (But GV's StrAllocVsprintf() is not, of course.) Ciao! --steffen | |Der Kragenbaer,The moon bear, |der holt sich munter he cheerfully and one by one |einen nach dem anderen runter wa.ks himself off |(By Robert Gernhardt)
Re: [Lynx-dev] SOCK5 crash+fix
On Thu, Mar 10, 2022 at 07:18:46PM -0500, Thomas Dickey wrote: > On Thu, Mar 10, 2022 at 07:22:21PM +0100, Steffen Nurpmeso wrote: > > Hello. > > > > I was the original author of the $SOCKS5_PROXY patch, but for one > > Thomas Dickey changed the C enormously, and i also have forgotten > > anything about it, but you noting it made me look. > > sorry - I didn't notice this at the time, but it was in Steffen's patch: https://github.com/ThomasDickey/lynx-snapshots/commit/3f9967cb255c102f7c0b09536fb6665392b013c6 -- Thomas E. Dickey https://invisible-island.net ftp://ftp.invisible-island.net signature.asc Description: PGP signature
Re: [Lynx-dev] SOCK5 crash+fix
Gisle Vanem wrote in <250c80e1-7697-3981-7f0e-42be642f7...@gmail.com>: |>> HTSprintf0 expects to have the address of a pointer in which it |>> can return a pointer to newly-allocated memory. |> |> 'HTSprintf0(NULL,..)' looks okay to me, but crashes |> on some condition anyway. Some Runtime-Checks that gets |> confused (?). | |I think I found the cause of it: | |--- orig/WWW/Library/Implementation/HTString.c 2021-06-09 22:16:06 |+++ WWW/Library/Implementation/HTString.c 2022-03-11 13:48:17 | char *dst_ptr = *pstr; | const char *format = fmt; |- char *dst_ptr = *pstr; // line 680 |+ char *dst_ptr = pstr ? *pstr : NULL; Oh i see. My bad not having looked deeply into this. Seems vasprintf stuff is everywhere i go. --End of <250c80e1-7697-3981-7f0e-42be642f7...@gmail.com> --steffen | |Der Kragenbaer,The moon bear, |der holt sich munter he cheerfully and one by one |einen nach dem anderen runter wa.ks himself off |(By Robert Gernhardt)
Re: [Lynx-dev] SOCK5 crash+fix
Gisle Vanem dixit: >> 'HTSprintf0(NULL,..)' looks okay to me, but crashes >> on some condition anyway. Some Runtime-Checks that gets >> confused (?). > > I think I found the cause of it: > > --- orig/WWW/Library/Implementation/HTString.c 2021-06-09 22:16:06 > +++ WWW/Library/Implementation/HTString.c 2022-03-11 13:48:17 >char *dst_ptr = *pstr; >const char *format = fmt; > - char *dst_ptr = *pstr; // line 680 > + char *dst_ptr = pstr ? *pstr : NULL; Ah! This looks better, indeed. bye, //mirabilos -- Gestern Nacht ist mein IRC-Netzwerk explodiert. Ich hatte nicht damit gerechnet, darum bin ich blutverschmiert… wer konnte ahnen, daß SIE so reagier’n… gestern Nacht ist mein IRC-Netzwerk explodiert~~~ (as of 2021-06-15 The MirOS Project temporarily reconvenes on OFTC)
Re: [Lynx-dev] SOCK5 crash+fix
HTSprintf0 expects to have the address of a pointer in which it can return a pointer to newly-allocated memory. 'HTSprintf0(NULL,..)' looks okay to me, but crashes on some condition anyway. Some Runtime-Checks that gets confused (?). I think I found the cause of it: --- orig/WWW/Library/Implementation/HTString.c 2021-06-09 22:16:06 +++ WWW/Library/Implementation/HTString.c 2022-03-11 13:48:17 char *dst_ptr = *pstr; const char *format = fmt; - char *dst_ptr = *pstr; // line 680 + char *dst_ptr = pstr ? *pstr : NULL;
Re: [Lynx-dev] SOCK5 crash+fix
Anyway, this works better.
int len = snprintf(NULL, 0, gettext("(for %s at %s) SOCKS5"), protocol,
socks5_host);
socks5_protocol = malloc (len+1);
if (!socks5_protocol)
outofmem(__FILE__, "malloc");
snprintf(socks5_buf, len, gettext("(for %s at %s) SOCKS5"), protocol,
socks5_host);
protocol = socks5_protocol;
A bit hasty, should be:
snprintf(socks5_protocol, len, gettext("(for %s at %s) SOCKS5"), protocol,
socks5_host);
--
--gv
Re: [Lynx-dev] SOCK5 crash+fix
Thomas Dickey wrote:
Subject: Re: [Lynx-dev] ..on SOCKS5 support
...
+p1 = NULL;
+
+protocol = HTSprintf0(NULL, gettext("(for %s at %s) SOCKS5"),
+protocol, socks5_host);
+}
+#ifndef INET6
HTSprintf0 expects to have the address of a pointer in which it
can return a pointer to newly-allocated memory.
'HTSprintf0(NULL,..)' looks okay to me, but crashes
on some condition anyway. Some Runtime-Checks that gets
confused (?).
Anyway, this works better.
int len = snprintf(NULL, 0, gettext("(for %s at %s) SOCKS5"), protocol,
socks5_host);
socks5_protocol = malloc (len+1);
if (!socks5_protocol)
outofmem(__FILE__, "malloc");
snprintf(socks5_buf, len, gettext("(for %s at %s) SOCKS5"), protocol,
socks5_host);
protocol = socks5_protocol;
and I'm happy with that.
--
--gv
Re: [Lynx-dev] SOCK5 crash+fix
Thomas Dickey wrote in
<20220311001846.ga6...@prl-debianold-64.jexium-island.net>:
|On Thu, Mar 10, 2022 at 07:22:21PM +0100, Steffen Nurpmeso wrote:
|> Hello.
|>
|> I was the original author of the $SOCKS5_PROXY patch, but for one
|> Thomas Dickey changed the C enormously, and i also have forgotten
|> anything about it, but you noting it made me look.
|
|sorry - I didn't notice this at the time, but it was in Steffen's patch:
...
|+protocol = HTSprintf0(NULL, gettext("(for %s at %s) SOCKS5"), \
|
|+protocol, socks5_host); \
|
|+} \
|
|
|+#ifndef INET6
|
|HTSprintf0 expects to have the address of a pointer in which it
|can return a pointer to newly-allocated memory.
|
|(the pointer is normally set to NULL by the caller).
Hm, but this is only optional, and we do not need it there since
we use the return value directly, and only.
No no, not that.
And by "changed the C enormously" i meant the replacement of jumps
and such .. i tend to come in problems with spacy and stretchy
styles, i loose grip looking at them.
I apologize for not using NETWRITE; it was likely that i searched
around in this file and only found a direct write() in
setup_nsl_fork(). And it does not make a difference on any system
i use, so no chance to see the problem.
--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)
Re: [Lynx-dev] SOCK5 crash+fix
On Thu, Mar 10, 2022 at 07:22:21PM +0100, Steffen Nurpmeso wrote:
> Hello.
>
> I was the original author of the $SOCKS5_PROXY patch, but for one
> Thomas Dickey changed the C enormously, and i also have forgotten
> anything about it, but you noting it made me look.
sorry - I didn't notice this at the time, but it was in Steffen's patch:
Date: Sun, 15 Sep 2019 00:04:18 +0200
From: Steffen Nurpmeso
To: Thomas Dickey
Cc: lynx-dev@nongnu.org
Subject: Re: [Lynx-dev] ..on SOCKS5 support
...
+p1 = NULL;
+
+protocol = HTSprintf0(NULL, gettext("(for %s at %s) SOCKS5"),
+protocol, socks5_host);
+}
+#ifndef INET6
HTSprintf0 expects to have the address of a pointer in which it
can return a pointer to newly-allocated memory.
(the pointer is normally set to NULL by the caller).
--
Thomas E. Dickey
https://invisible-island.net
ftp://ftp.invisible-island.net
signature.asc
Description: PGP signature
Re: [Lynx-dev] SOCK5 crash+fix
Steffen Nurpmeso dixit:
> |socks5_protocol = HTSprintf0(NULL,
> | gettext("(for %s at %s) SOCKS5"),
> | protocol, socks5_host);
> |
> |A NULL-ptr read which I fail to understand.
Which is NULL here, the target variable (does HTSprintf0 not
allocate) or one of the sources (does HTSprintf0, unlike some
OSes’ snprintf, not permit NULL for %s and replace it with the
string "(null)"), in which case the patch is also wrong because
not all OSes do that, and then it needs to be:
snprintf(socks5_buf, sizeof(socks5_buf),
gettext("(for %s at %s) SOCKS5"), protocol ? protocol : "(null)",
socks5_host ? socks5_host : "(null)");
> |char socks5_buf [1000];
Large buffers on the stack are possibly harmful though.
> |+ (SOCKET_ERRNO == EINPROGRESS || SOCKET_ERRNO == 112
This can’t be right.
>I do not know what 112 is, but i have no idea of Windows, what
WSA* errnos are in five-digit range. WSAEINPROGRESS is 10036,
for example. Magic numbers are bad, too; this probably needs
explaining.
>+#ifdef _WINDOWS
>+ || SOCKET_ERRNO == 112
> #endif
As I said above, almost certainly wrong.
bye,
//mirabilos
--
Solange man keine schmutzigen Tricks macht, und ich meine *wirklich*
schmutzige Tricks, wie bei einer doppelt verketteten Liste beide
Pointer XORen und in nur einem Word speichern, funktioniert Boehm ganz
hervorragend. -- Andreas Bogk über boehm-gc in d.a.s.r
Re: [Lynx-dev] SOCK5 crash+fix
Hello.
I was the original author of the $SOCKS5_PROXY patch, but for one
Thomas Dickey changed the C enormously, and i also have forgotten
anything about it, but you noting it made me look.
which had a
Gisle Vanem wrote in
<8906d293-f9ff-3d11-89d7-4bdeb944a...@gmail.com>:
|While trying Lynx with Tor's SOCK5 proxy:
| lynx -dump -socks5_proxy=localhost:9050
| https://www.bbcweb3hytmzhn5d532owbu6oqadra5z3ar726vq5kgwwn6aucdccrad\
| .onion/
|
|(the BBC Homepage), I got a strange crash for this code:
|socks5_protocol = HTSprintf0(NULL,
| gettext("(for %s at %s) SOCKS5"),
| protocol, socks5_host);
|
|A NULL-ptr read which I fail to understand.
Ok you mean that function may return NULL in out-of-memory and
thus this condition should be checked.
Hm, but HTAlloc() bails fatally in this condition?
if (ptr == 0)
outofmem(__FILE__, "HTAlloc");
|But simply replacing with:
|char socks5_buf [1000];
|...
|snprintf(socks5_buf, sizeof(socks5_buf),
| gettext("(for %s at %s) SOCKS5"), protocol, socks5_host);
|protocol = socks5_buf;
..So this i do not buy.
|plus some other patches to HTTCP.c (attached), Lynx+Tor
|works on Windows-10. The diff is against the 2.9.0dev.10
|version. Latest I believe (?)
...
|--- orig/WWW/Library/Implementation/HTTCP.c 2021-06-09 01:44:43
|+++ WWW/Library/Implementation/HTTCP.c 2022-03-10 14:11:59
|@@ -1828,9 +1828,9 @@
| char *socks5_host = NULL;
| unsigned socks5_host_len = 0;
| int socks5_port;
|+char socks5_buf [1000];
| const char *socks5_orig_url;
| char *socks5_new_url = NULL;
|-char *socks5_protocol = NULL;
| int status = HT_OK;
| char *line = NULL;
| char *p1 = NULL;
|@@ -1888,10 +1888,9 @@
| HTSACat(&socks5_new_url, socks5_proxy);
| url = socks5_new_url;
|
|- socks5_protocol = HTSprintf0(NULL,
|- gettext("(for %s at %s) SOCKS5"),
|- protocol, socks5_host);
|- protocol = socks5_protocol;
|+ snprintf(socks5_buf, sizeof(socks5_buf),
|+ gettext("(for %s at %s) SOCKS5"), protocol, socks5_host);
|+ protocol = socks5_buf;
|}
| #ifndef INET6
| /*
With the above said, this is unrelated and worse or worse :)
|@@ -2032,8 +2031,10 @@
| * write service procedure. This will be
| * the normal case.
| */
|+ CTRACE((tfp, "connect(): status: %d, SOCK_ERRNO: %d\n", status, \
|SOCKET_ERRNO));
|+
| if ((status < 0) &&
|- (SOCKET_ERRNO == EINPROGRESS
|+ (SOCKET_ERRNO == EINPROGRESS || SOCKET_ERRNO == 112
| #ifdef EAGAIN
||| SOCKET_ERRNO == EAGAIN
| #endif
I do not know what 112 is, but i have no idea of Windows, what
i know was 3.* and 95 B. You are surely right.
|@@ -2091,7 +2092,7 @@
|* If we suspend, then it is possible that select will be
|* interrupted. Allow for this possibility. - JED
|*/
|- if ((ret == -1) && (errno == EINTR))
|+ if ((ret == -1) && (SOCKET_ERRNO == EINTR))
| continue;
|
| #ifdef SOCKET_DEBUG_TRACE
This looks right to me given that SOCKER_ERRNO != errno there.
|@@ -2273,7 +2281,7 @@
| pbuf[0] = 0x05; /* VER: protocol version: X'05' */
| pbuf[1] = 0x01; /* NMETHODS: 1 */
| pbuf[2] = 0x00; /* METHOD: X'00' NO AUTHENTICATION REQUIRED \
| */
|- if (write(*s, pbuf, 3) != 3) {
|+ if (NETWRITE(*s, pbuf, 3) != 3) {
Interesting! This ... seems right then too?
I did not know this!
...
The below patch against [d84fd6821970236a99d0d38b1df98c74463cec02]
aka v2-9-0dev_10a, does it work for you too?
(Note i have not even compile-tested it, but .. should!!!)
diff --git a/WWW/Library/Implementation/HTTCP.c
b/WWW/Library/Implementation/HTTCP.c
index c4d648acde..99f4620f2f 100644
--- a/WWW/Library/Implementation/HTTCP.c
+++ b/WWW/Library/Implementation/HTTCP.c
@@ -2036,6 +2036,9 @@ int HTDoConnect(const char *url,
(SOCKET_ERRNO == EINPROGRESS
#ifdef EAGAIN
|| SOCKET_ERRNO == EAGAIN
+#endif
+#ifdef _WINDOWS
+|| SOCKET_ERRNO == 112
#endif
)) {
struct timeval select_timeout;
@@ -2091,7 +2094,7 @@ int HTDoConnect(const char *url,
* If we suspend, then it is possible that select will be
* interrupted. Allow for this possibility. - JED
*/
- if ((ret == -1) && (errno == EINTR))
+ if ((ret == -1) && (SOCKET_ERRNO == EINTR))
continue;
#ifdef SOCKET_DEBUG_TRACE
@@ -2273,7 +2276,7 @@ int HTDoConnect(const char *url,
pbuf[0] = 0x05; /* VER: protocol version: X'05' */
pbuf[1] = 0x01; /* NMETHODS: 1 */
pbuf[2] = 0x00; /* METHOD: X'00' NO AUTHENTICATION REQUIRED */
- if (write(*s, pbuf
[Lynx-dev] SOCK5 crash+fix
While trying Lynx with Tor's SOCK5 proxy:
lynx -dump -socks5_proxy=localhost:9050
https://www.bbcweb3hytmzhn5d532owbu6oqadra5z3ar726vq5kgwwn6aucdccrad.onion/
(the BBC Homepage), I got a strange crash for this code:
socks5_protocol = HTSprintf0(NULL,
gettext("(for %s at %s) SOCKS5"),
protocol, socks5_host);
A NULL-ptr read which I fail to understand.
But simply replacing with:
char socks5_buf [1000];
...
snprintf(socks5_buf, sizeof(socks5_buf),
gettext("(for %s at %s) SOCKS5"), protocol, socks5_host);
protocol = socks5_buf;
plus some other patches to HTTCP.c (attached), Lynx+Tor
works on Windows-10. The diff is against the 2.9.0dev.10
version. Latest I believe (?)
--
--gv
--- orig/WWW/Library/Implementation/HTTCP.c 2021-06-09 01:44:43
+++ WWW/Library/Implementation/HTTCP.c 2022-03-10 14:11:59
@@ -1828,9 +1828,9 @@
char *socks5_host = NULL;
unsigned socks5_host_len = 0;
int socks5_port;
+char socks5_buf [1000];
const char *socks5_orig_url;
char *socks5_new_url = NULL;
-char *socks5_protocol = NULL;
int status = HT_OK;
char *line = NULL;
char *p1 = NULL;
@@ -1888,10 +1888,9 @@
HTSACat(&socks5_new_url, socks5_proxy);
url = socks5_new_url;
- socks5_protocol = HTSprintf0(NULL,
-gettext("(for %s at %s) SOCKS5"),
-protocol, socks5_host);
- protocol = socks5_protocol;
+ snprintf(socks5_buf, sizeof(socks5_buf),
+gettext("(for %s at %s) SOCKS5"), protocol, socks5_host);
+ protocol = socks5_buf;
}
#ifndef INET6
/*
@@ -2032,8 +2031,10 @@
* write service procedure. This will be
* the normal case.
*/
+ CTRACE((tfp, "connect(): status: %d, SOCK_ERRNO: %d\n", status,
SOCKET_ERRNO));
+
if ((status < 0) &&
- (SOCKET_ERRNO == EINPROGRESS
+ (SOCKET_ERRNO == EINPROGRESS || SOCKET_ERRNO == 112
#ifdef EAGAIN
|| SOCKET_ERRNO == EAGAIN
#endif
@@ -2091,7 +2092,7 @@
* If we suspend, then it is possible that select will be
* interrupted. Allow for this possibility. - JED
*/
- if ((ret == -1) && (errno == EINTR))
+ if ((ret == -1) && (SOCKET_ERRNO == EINTR))
continue;
#ifdef SOCKET_DEBUG_TRACE
@@ -2273,7 +2281,7 @@
pbuf[0] = 0x05; /* VER: protocol version: X'05' */
pbuf[1] = 0x01; /* NMETHODS: 1 */
pbuf[2] = 0x00; /* METHOD: X'00' NO AUTHENTICATION REQUIRED */
- if (write(*s, pbuf, 3) != 3) {
+ if (NETWRITE(*s, pbuf, 3) != 3) {
goto report_system_err;
} else if (HTDoRead(*s, pbuf, 2) != 2) {
goto report_system_err;
@@ -2298,7 +2306,7 @@
memcpy(&pbuf[i], (unsigned char *) &x, sizeof x);
i += (unsigned) sizeof(x);
}
- if ((size_t) write(*s, pbuf, i) != i) {
+ if ((size_t) NETWRITE(*s, pbuf, i) != i) {
goto report_system_err;
} else if ((unsigned) HTDoRead(*s, pbuf, 4) != 4) {
goto report_system_err;
@@ -2396,7 +2404,6 @@
cleanup:
if (socks5_proxy != NULL) {
FREE(socks5_new_url);
- FREE(socks5_protocol);
FREE(socks5_host);
}
FREE(host);
@@ -2534,7 +2541,7 @@
break;
}
#else /* UNIX */
- result = SOCKET_READ(fildes, buf, nbyte);
+ result = NETREAD(fildes, buf, nbyte);
#endif /* !UNIX */
#endif /* UCX && VAXC */
}
