Обсуждение: Bug #925: typing error in src/backend/libpq/be-secure.c ???

Sergey N. Yatskevich (syatskevich@n21lab.gosniias.msk.ru) reports a bug with a severity of 4
The lower the number the more severe it is.

Short Description
typing error in src/backend/libpq/be-secure.c ???

Long Description
In src/backend/libpq/be-secure.c: secure_write
on SSL_ERROR_WANT_WRITE call secure_read instead
secure_write again. May be is this a typing error?


Sample Code


No file was uploaded with this report

Yep, typo.  Patched to CVS current and backpatched to 7.3.X.

---------------------------------------------------------------------------

pgsql-bugs@postgresql.org wrote:
> Sergey N. Yatskevich (syatskevich@n21lab.gosniias.msk.ru) reports a bug with a severity of 4
> The lower the number the more severe it is.
>
> Short Description
> typing error in src/backend/libpq/be-secure.c ???
>
> Long Description
> In src/backend/libpq/be-secure.c: secure_write
> on SSL_ERROR_WANT_WRITE call secure_read instead
> secure_write again. May be is this a typing error?
>
>
> Sample Code
>
>
> No file was uploaded with this report
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/libpq/be-secure.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/libpq/be-secure.c,v
retrieving revision 1.26
diff -c -c -r1.26 be-secure.c
*** src/backend/libpq/be-secure.c    14 Feb 2003 00:18:41 -0000    1.26
--- src/backend/libpq/be-secure.c    29 Mar 2003 03:55:09 -0000
***************
*** 338,344 ****
                  port->count += n;
                  break;
              case SSL_ERROR_WANT_WRITE:
!                 n = secure_read(port, ptr, len);
                  break;
              case SSL_ERROR_SYSCALL:
                  if (n == -1)
--- 338,344 ----
                  port->count += n;
                  break;
              case SSL_ERROR_WANT_WRITE:
!                 n = secure_write(port, ptr, len);
                  break;
              case SSL_ERROR_SYSCALL:
                  if (n == -1)

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Yep, typo.  Patched to CVS current and backpatched to 7.3.X.

I think this fix is exactly backward.  Why would SSL_write need to
return ERROR_WANT_WRITE?  It couldn't.  The correct fix is that
SSL_write might return ERROR_WANT_READ, for which reading would be
the right response.

BTW the real problem, both here and elsewhere in this file, is the
lack of a "default: elog-out" case in the switch statements.  This
code will simply break if any unexpected case occurs.

            regards, tom lane

Actually, the docs say SSL_read/write can error needing READ or WRITE:

    http://www.openssl.org/docs/ssl/SSL_read.html#

The SSL_write docs are the same.  I have applied the following patch to
handle both new cases.  Does this help the SSL test program you have?

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Yep, typo.  Patched to CVS current and backpatched to 7.3.X.
>
> I think this fix is exactly backward.  Why would SSL_write need to
> return ERROR_WANT_WRITE?  It couldn't.  The correct fix is that
> SSL_write might return ERROR_WANT_READ, for which reading would be
> the right response.
>
> BTW the real problem, both here and elsewhere in this file, is the
> lack of a "default: elog-out" case in the switch statements.  This
> code will simply break if any unexpected case occurs.
>
>             regards, tom lane
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/libpq/be-secure.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/libpq/be-secure.c,v
retrieving revision 1.27
diff -c -c -r1.27 be-secure.c
*** src/backend/libpq/be-secure.c    29 Mar 2003 03:56:44 -0000    1.27
--- src/backend/libpq/be-secure.c    29 Mar 2003 04:59:01 -0000
***************
*** 285,290 ****
--- 285,293 ----
              case SSL_ERROR_WANT_READ:
                  n = secure_read(port, ptr, len);
                  break;
+             case SSL_ERROR_WANT_WRITE:
+                 n = secure_write(port, ptr, len);
+                 break;
              case SSL_ERROR_SYSCALL:
                  if (n == -1)
                      elog(COMMERROR, "SSL SYSCALL error: %s", strerror(errno));
***************
*** 336,341 ****
--- 339,347 ----
          {
              case SSL_ERROR_NONE:
                  port->count += n;
+                 break;
+             case SSL_ERROR_WANT_READ:
+                 n = secure_read(port, ptr, len);
                  break;
              case SSL_ERROR_WANT_WRITE:
                  n = secure_write(port, ptr, len);

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Actually, the docs say SSL_read/write can error needing READ or WRITE:
>     http://www.openssl.org/docs/ssl/SSL_read.html#

Only when using a non-blocking BIO, which the backend had better not be
doing.  But a few lines of useless code aren't a big deal...

> The SSL_write docs are the same.  I have applied the following patch to
> handle both new cases.  Does this help the SSL test program you have?

Where is the "default:" case to preserve sanity when the SSL call returns
an error code other than the ones the switch knows about?  ISTM the lack
of this defense *is* a big deal.

            regards, tom lane