On Thu, Apr 18, 2024 at 10:51:34PM -0400, Allan Streib wrote:
> I looked at the files/ssl-openssl.c in this port. In the
> ssl_openssl_write function, if there is an error other than
> SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE then the execution falls
> through to return 0.
>
> I changed the code to include the actual result from the SSL_get_error
> function in the debug message, and return -1 instead of falling through
> to return 0. The ssl_gnutls_write function does that, and it seems quite
> similar. Not sure which one is based upon the other. ssl_gnutls_write
> also sets errno = EIO in this situation, so I added that as well. The
> ssl_openssl_read function is quite similar, and though I have not seen
> any errors from that, I made the same changes there.
>
> This has made a big improvement, I still see a few errors, maybe 10 or
> 12 at a time, which are all "openssl: send failed: 5" but not the many
> thousands that I saw before. The cpu load of the finch process stays
> quite low.
I am not entirely sure why this code ignores errors and sets the return
value to 0. The read and write handlers in the purple API are strange
since they propagate int to size_t (not ssize_t!) and then further up
the stack, things truncate that size_t to an int again. So the return
-1 becomes SIZE_MAX and then -1 again.
With your diff, "openssl: send failed: 5" tells us that SSL_get_error()
returned SSL_ERROR_SYSCALL. In most cases, this means an IO error
occurred on the socket (it could also be an unexpected EOF), so setting
errno = EIO and return -1 is mostly appropriate. I haven't found a caller
specifically checking for EIO. The important bit is that the errno is
distinct from EAGAIN.
> I tried to make a diff for this. It seems to fix my problem, but don't
> know if introduces any others, so I offer it for what it's worth....
In sum, I think this diff is closer to correct than what is currently in
the tree. As my only real suggestion: you could consider handling the
SSL_ERROR_ZERO_RETURN by returning 0, but I don't think it changes
anything in the busy loop you're encountering. And yes, I think the
approach of handling read and write the same way is the way to go.
>
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/net/pidgin/Makefile,v
> retrieving revision 1.161
> diff -u -p -u -r1.161 Makefile
> --- Makefile 27 Sep 2023 14:18:28 -0000 1.161
> +++ Makefile 19 Apr 2024 02:21:26 -0000
> @@ -3,6 +3,7 @@ COMMENT-finch= multi-protocol instant me
> COMMENT-libpurple= multi-protocol instant messaging library
>
> VERSION= 2.14.12
> +REVISION= 0
> DISTNAME= pidgin-${VERSION}
> PKGNAME-main= pidgin-${VERSION}
> PKGNAME-finch= finch-${VERSION}
> Index: files/ssl-openssl.c
> ===================================================================
> RCS file: /cvs/ports/net/pidgin/files/ssl-openssl.c,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 ssl-openssl.c
> --- files/ssl-openssl.c 11 Mar 2022 19:46:55 -0000 1.11
> +++ files/ssl-openssl.c 19 Apr 2024 02:21:26 -0000
> @@ -203,8 +203,9 @@ ssl_openssl_read(PurpleSslConnection *gs
> return (-1);
> }
>
> - purple_debug_error("openssl", "receive failed: %zi\n", s);
> - s = 0;
> + purple_debug_error("openssl", "receive failed: %d\n", ret);
> + errno = EIO;
> + return (-1);
> }
>
> return (s);
> @@ -229,8 +230,9 @@ ssl_openssl_write(PurpleSslConnection *g
> return (-1);
> }
>
> - purple_debug_error("openssl", "send failed: %zi\n", s);
> - s = 0;
> + purple_debug_error("openssl", "send failed: %d\n", ret);
> + errno = EIO;
> + return (-1);
> }
>
> return (s);
>
>
> --
> Allan Streib
> Luddy School of Informatics, Computing, and Engineering
> Indiana University
>
