Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firefox 3 and https connections #2892

Closed
michaelrsweet opened this issue Aug 1, 2008 · 10 comments
Closed

Firefox 3 and https connections #2892

michaelrsweet opened this issue Aug 1, 2008 · 10 comments
Labels
Milestone

Comments

@michaelrsweet
Copy link
Collaborator

@michaelrsweet michaelrsweet commented Aug 1, 2008

Version: 1.3.7
CUPS.org User: twaugh.redhat

I know you've seen bug reports about this before and closed them, but I think I've uncovered the cause of it, and it is CUPS after all.

In Firefox 3, SSL traffic is sent in fewer packets than in previous versions, so clicking on 'Continue' in the CUPS web interface when adding a printer may only send one single large packet.

What I have observed is that when data is available to read on a connection, and this is SSL data, it will read in a buffer full and process it. However, when cupsdReadClient finishes, the scheduler will not wake up until the next time poll() says there is data to read.

The problem is that it has already been read, by the SSL library (gnutls, in the case I'm examining). The data is sitting waiting to be discovered by gnutls_record_check_pending(), but this function is never called after cupsdReadClient returns, at least not until the user clicks 'Stop' and the connection is closed.

At that point, the scheduler wakes up and processes the data -- but by then it's too late.

Attached is a function that wraps cupsdReadClient() and just calls it again if more data is available. This works around the problem for me.

Full steps for reproduction:

Build CUPS against gnutls (not sure if this is relevant)
Using firefox 3, go to https://localhost:631
Click 'Add Printer'
Enter the name as 'test' and click Continue
Select AppSocket/HP JetDirect for the device and click Continue
Enter socket://hostname as the URI and click Continue
Select any make from the list and click Continue

At this point, the browser is waiting for data, and so is CUPS. With the patch applied, CUPS responds with a model list page.

@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 1, 2008

CUPS.org User: mike

OK, right idea, but wrong implementation.

We really only need to update the data reading code in cupsdReadClient to use the equivalent of httpCheck instead of only looking at con->http.used.
(CUPS doesn't support HTTP pipelining so we don't need to handle queuing up multiple requests in a single packet...)

Patch coming up...

@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 1, 2008

CUPS.org User: mike

Here you go... I've tested this with FF3 on Mac OS X (CDSA) and RHEL5 (OpenSSL), but not a system that uses GNU TLS. However, I did copy the code from http_wait() (which is what httpCheck() uses), so I'm pretty confident that GNU TLS installs will be fixed as well.

Let me know how your testing goes...

@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 1, 2008

CUPS.org User: mike

and here is a patch against 1.3.x since the 1.4.x patch didn't apply cleanly...

@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 1, 2008

CUPS.org User: twaugh.redhat

The 1.3.x version works fine here with gnutls. I haven't tested the trunk version.

Thanks!

@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 2, 2008

CUPS.org User: mike

Fixed in Subversion repository.

@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 2, 2008

"cups-firefox3.patch":

diff -up cups-1.3.7/scheduler/client.c.firefox3 cups-1.3.7/scheduler/client.c
--- cups-1.3.7/scheduler/client.c.firefox3 2008-08-01 16:58:05.000000000 +0100
+++ cups-1.3.7/scheduler/client.c 2008-08-01 16:58:48.000000000 +0100
@@ -772,8 +772,8 @@ cupsdFlushHeader(cupsd_client_t con) /

  • 'cupsdReadClient()' - Read data from a client.
    */

-void
-cupsdReadClient(cupsd_client_t con) / I - Client to read from /
+static void
+read_client (cupsd_client_t *con) /
I - Client to read from /
{
char line[32768], /
Line from client... /
operation[64], /
Operation code from socket /
@@ -2274,6 +2274,15 @@ cupsdReadClient(cupsd_client_t *con) /

cupsdCloseClient(con);
}

+void
+cupsdReadClient(cupsd_client_t *con)
+{

  • do
  • {
  • read_client (con);
  • } while (httpCheck (con));
    +}

/*

  • 'cupsdSendCommand()' - Send output from a command via HTTP.
@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 2, 2008

"str2892.patch":

Index: scheduler/client.c

--- scheduler/client.c (revision 7817)
+++ scheduler/client.c (working copy)
@@ -29,6 +29,7 @@

  • cupsdWriteClient() - Write data to a client as needed.

  • check_if_modified() - Decode an "If-Modified-Since" line.

  • compare_clients() - Compare two client connections.

  • * data_ready() - Check whether data is available from a client.

  • encrypt_client() - Enable encryption for the client...

  • get_cdsa_certificate() - Convert a keychain name into the CFArrayRef

  •            required by SSLSetCertificate.
    

    @@ -89,6 +90,7 @@
    struct stat filestats);
    static int compare_clients(cupsd_client_t *a, cupsd_client_t *b,
    void *data);
    +static int data_ready(cupsd_client_t *con);
    #ifdef HAVE_SSL
    static int encrypt_client(cupsd_client_t *con);
    #endif /
    HAVE_SSL */
    @@ -1046,8 +1048,7 @@
    */

     while ((status = httpUpdate(HTTP(con))) == HTTP_CONTINUE)
    
    • if (con->http.used == 0 ||
      
    •     !memchr(con->http.buffer, '\n', con->http.used))
      
    • if (!data_ready(con))
      
      break;

    if (status != HTTP_OK && status != HTTP_CONTINUE)
    @@ -1946,7 +1947,7 @@
    }
    }
    }

    • while (con->http.state == HTTP_PUT_RECV && con->http.used > 0);
    • while (con->http.state == HTTP_PUT_RECV && data_ready(con));

    if (con->http.state == HTTP_WAITING)
    {
    @@ -2121,7 +2122,7 @@
    }
    }
    }

    • while (con->http.state == HTTP_POST_RECV && con->http.used > 0);
    • while (con->http.state == HTTP_POST_RECV && data_ready(con));

    if (con->http.state == HTTP_POST_SEND)
    {
    @@ -3006,7 +3007,39 @@
    }

+/*

  • * 'data_ready()' - Check whether data is available from a client.
  • /
    +
    +static int /
    O - 1 if data is ready, 0 otherwise /
    +data_ready(cupsd_client_t *con) /
    I - Client */
    +{
  • if (con->http.used > 0)
  • return (1);
    #ifdef HAVE_SSL
  • else if (con->http.tls)
  • {
    +# ifdef HAVE_LIBSSL
  • if (SSL_pending((SSL *)(con->http.tls)))
  •  return (1);
    
    +# elif defined(HAVE_GNUTLS)
  • if (gnutls_record_check_pending(((http_tls_t *)(con->http.tls))->session))
  •  return (1);
    
    +# elif defined(HAVE_CDSASSL)
  • size_t bytes; /* Bytes that are available */
  • if (!SSLGetBufferedReadSize(((http_tls_t *)(con->http.tls))->session,
  •                            &bytes) && bytes > 0)
    
  •  return (1);
    
    +# endif /* HAVE_LIBSSL */
  • }
    +#endif /* HAVE_SSL */
  • return (0);
    +}

+#ifdef HAVE_SSL
/*

  • 'encrypt_client()' - Enable encryption for the client...
    */
@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 2, 2008

"str2892-1.3.patch":

Index: scheduler/client.c

--- scheduler/client.c (revision 7820)
+++ scheduler/client.c (working copy)
@@ -28,6 +28,7 @@

  • cupsdUpdateCGI() - Read status messages from CGI scripts and programs.

  • cupsdWriteClient() - Write data to a client as needed.

  • check_if_modified() - Decode an "If-Modified-Since" line.

  • * data_ready() - Check whether data is available from a client.

  • encrypt_client() - Enable encryption for the client...

  • get_cdsa_certificate() - Convert a keychain name into the CFArrayRef

  •            required by SSLSetCertificate.
    

    @@ -83,6 +84,7 @@

    static int check_if_modified(cupsd_client_t con,
    struct stat *filestats);
    +static int data_ready(cupsd_client_t *con);
    #ifdef HAVE_SSL
    static int encrypt_client(cupsd_client_t *con);
    #endif /
    HAVE_SSL */
    @@ -989,8 +991,7 @@
    */

     while ((status = httpUpdate(HTTP(con))) == HTTP_CONTINUE)
    
    • if (con->http.used == 0 ||
      
    •     !memchr(con->http.buffer, '\n', con->http.used))
      
    • if (!data_ready(con))
      
      break;

    if (status != HTTP_OK && status != HTTP_CONTINUE)
    @@ -1889,7 +1890,7 @@
    }
    }
    }

    • while (con->http.state == HTTP_PUT_RECV && con->http.used > 0);
    • while (con->http.state == HTTP_PUT_RECV && data_ready(con));

    if (con->http.state == HTTP_WAITING)
    {
    @@ -2064,7 +2065,7 @@
    }
    }
    }

    • while (con->http.state == HTTP_POST_RECV && con->http.used > 0);
    • while (con->http.state == HTTP_POST_RECV && data_ready(con));

    if (con->http.state == HTTP_POST_SEND)
    {
    @@ -2914,7 +2915,39 @@
    }

+/*

  • * 'data_ready()' - Check whether data is available from a client.
  • /
    +
    +static int /
    O - 1 if data is ready, 0 otherwise /
    +data_ready(cupsd_client_t *con) /
    I - Client */
    +{
  • if (con->http.used > 0)
  • return (1);
    #ifdef HAVE_SSL
  • else if (con->http.tls)
  • {
    +# ifdef HAVE_LIBSSL
  • if (SSL_pending((SSL *)(con->http.tls)))
  •  return (1);
    
    +# elif defined(HAVE_GNUTLS)
  • if (gnutls_record_check_pending(((http_tls_t *)(con->http.tls))->session))
  •  return (1);
    
    +# elif defined(HAVE_CDSASSL)
  • size_t bytes; /* Bytes that are available */
  • if (!SSLGetBufferedReadSize(((http_tls_t *)(con->http.tls))->session,
  •                            &bytes) && bytes > 0)
    
  •  return (1);
    
    +# endif /* HAVE_LIBSSL */
  • }
    +#endif /* HAVE_SSL */
  • return (0);
    +}

+#ifdef HAVE_SSL
/*

  • 'encrypt_client()' - Enable encryption for the client...
    */
@vgavgavga

This comment has been minimized.

Copy link

@vgavgavga vgavgavga commented Oct 16, 2018

I've got an old ubuntu Server with this Bug present. How do I patch my CUPS version with these files?

@michaelrsweet

This comment has been minimized.

Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Oct 16, 2018

@vgavgavga Contact the Ubuntu folks - there isn't anything we can tell you here, unfortunately...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.