Edge case with socket becoming NULL

Helge Heß me at helgehess.eu
Sun Aug 2 18:21:18 EDT 2009


On 02.08.2009, at 23:54, Joe Orton wrote:
>> code section, ne_request.c, Neon 0.28.5:
>> ---snip---
>> static int read_message_header(ne_request *req, char *buf, size_t
>> buflen)
>> {
>>    ssize_t n;
>>    ne_socket *sock = req->session->socket;
>>
>>    n = ne_sock_readline(sock, buf, buflen);
>> ---snap---
>>
>> I've seen the 'sock' ptr become NULL during debugging which then  
>> leads
>> to subsequent NULL-ptr deref.
>
> Are you calling ne_begin_request/ne_end_request manually, or via
> ne_request_dispatch?

Manually.

> Note that if you're calling ne_begin_request(), you must not call
> ne_end_request() if a ne_read_response_block() call fails - the API is
> perhaps not clear on this point.

Hm. I'll recheck that. Could be an issue. Couldn't  
ne_read_response_block set a flag which signals ne_end_request that it  
should perform as a noop?

We do:
- ne_begin_request
- ne_get_status
- ne_response_header_iterate
- ne_discard_response
- ne_end_request

Attached is a code exerpt, maybe you get the idea ...

Thanks,
   Helge

---snip---
   do {
     int statCode;
     stat = NULL;
     Z1DESTROY(lastNeonError);

     this->_ResetResponse();
     assert(this->response == NULL);
     this->response = new Z1HttpResponse();


     /* start request */

     if (this->connection != NULL) {
       if (!this->connection->WillSendRequest(this->request)) {
	if (this->log != NULL) {
	  this->log->PrintF("%s: WillSendRequest() stopped send",
			    "Z1HttpTransaction::Run");
	}
	return false;
       }
     }

     if (this->log != NULL) {
       this->log->PrintF("%s: Send %s %s",
         "Z1HttpTransaction::Run",
         Z1StringUTF8(this->request->Method()),
         Z1StringUTF8(this->request->Path()));
       this->request->Dump(this->log);
     }

     if (rq == NULL) {
       if (this->log != NULL) {
         this->log->PrintF("%s:   RePrepare request, was NULL",
           "Z1HttpTransaction::Run");
       }
       if (!this->Prepare()) {
         if (this->log != NULL) {
           this->log->PrintF("%s: ERROR failed to RePrepare request.",
             "Z1HttpTransaction::Run");
         }
         rc = NE_ERROR;
         break;
       }
     }
     assert(rq != NULL);
     if (rq == NULL) {
       rc = NE_ERROR;
       break;
     }

     rc = ne_begin_request(rq);
     if (rc != NE_OK) {
       /* this still happens, eg with SOGo Task-Delete which issues
        * 2 subsequent GETs. The second one fails with:
        *   1 and "Could not parse response status line"
        *
        * Easy to reproduce with SOGo and persistent connections,
        * just do a few moves.
        * ne_parse_statusline() gets this input (why 8859-1?):
        *   <?xml version="1.0" encoding="ISO-8859-1"?>
        * probably an old buffer?
        */
       /* Also seen this:
        *   "Could not resolve hostname `sogo-demo.inverse.ca': The  
requested
        *    name is valid and was found in the database, but it does  
not
        *    have the correct associated data being resolved for."
        * This seems to be Winsock Error WSANO_DATA aka 11004.
        */
       const char *ne = ne_get_error((ne_session *)this- 
 >ConnectionHandle());
       if (this->log != NULL) {
         this->log->PrintF("%s:   begin_request failed: %i: %s (close- 
retry=%i)",
           "Z1HttpTransaction::Run", rc, ne, closeRetryCount);
       }
       if (ne != NULL && strstr(ne, "Could not read status line") ==  
NULL)
         ne = NULL;
       if (ne != NULL && strstr(ne, "closed by server") == NULL)
         ne = NULL;

       this->_CloseConnection(); // better be sure, close it

       if (ne != NULL && closeRetryCount < 3) {
         closeRetryCount++;
         if (this->log != NULL)
           this->log->PrintF("%s:  retry after close ...",  
"Z1HttpTransaction::Run");
         continue; /* try again */
       }
       else if (ne != NULL) {
         if (this->log != NULL) {
           this->log->PrintF("%s:   NO retry, already tried %i times.",
             "Z1HttpTransaction::Run", closeRetryCount);
         }
       }
       else {
         if (this->log != NULL)
           this->log->PrintF("%s:   NO retry, other error.",  
"Z1HttpTransaction::Run");
       }

       break;
     }

     /* check status */

     stat     = ne_get_status(rq);
     statCode = stat->code;
     this->response->SetStatus(statCode);

     if (this->connection != NULL)
       this->connection->DidSendRequest(this->request);


     /* copy response headers into Z1HttpResponse */

     void *itCursor = NULL;
     const char *headerKey = NULL, *headerValue = NULL;

     // Google: Server: GFE/1.3
     while ((itCursor = ne_response_header_iterate((ne_request*)this- 
 >handle,
						  itCursor,
						  &headerKey, &headerValue)))
     {
       this->response->AddHeaderForKeyAndReleaseBoth
         (Z1String::New(headerValue), z1HeaderKey(headerKey));
     }
     this->response->Dump(this->log);

#if 0 // yes, this happens with iCal Server when the content is  
transferred chunked
     if (Z1IsEmpty(this->response- 
 >HeaderForKey(Z1HttpHeader_ContentLength))) {
       ne_request *nrq = (ne_request*)this->handle;
       const char *clen = ne_get_response_header(nrq, "content-length");
       if (this->log != NULL)
         this->log->PrintF("CLEN: %s", clen);
     }
#endif

     stat = NULL; // pointer into request, be sure we don't use it  
anymore

     /* read response */

     if (this->connection != NULL) {
       if (!this->connection->WillReceiveResponse(this->response)) {
         if (this->log != NULL) {
           this->log->PrintF("%s: WillReceiveResponse() stopped  
receive",
			      "Z1HttpTransaction::Run");
         }
         if (rq != NULL) ne_end_request(rq);

         /* close connection. We did not read the content, hence it  
become
          * b0rked. */
         this->_CloseConnection(); // better be sure, close it
         return false;
       }
     }

     if (this->log != NULL) {
       this->log->PrintF("%s:   receive response (status=%i)",
         "Z1HttpTransaction::Run", this->response->Status());
     }

     /* read response */

     // calls ne_read_response_block until it returns 0
     // returns != 0 on error
     this->readCount = 0;
     if (ne_discard_response(rq) == NE_OK /* SUCCESS */) {
       if (this->response != NULL && this->responseData != NULL) {
         this->response->SetContentAndRelease(this->responseData);
         this->responseData = NULL;

#if DEBUG
         double ratio = this->response->GetCompressionRatio();
         if (ratio > 1 && this->log != NULL) {
           this->log->PrintF("%s:   compression: %0.3f",
             "Z1HttpTransaction::Run", ratio);
         }
#endif
       }

       /* close request, if it returns NE_RETRY, retry ... */
       rc = ne_end_request(rq);

       if (rc != NE_OK && rc != NE_RETRY) {
         const char *nes = ne_get_error((ne_session *)this- 
 >ConnectionHandle());
         if (nes != NULL) { // copy, it might go away on con-close!
           assert(lastNeonError == NULL);
           lastNeonError = Z1String::New(nes);
         }
       }

       if (this->connection != NULL)
          this->connection->DidReceiveResponse(this->response);
     }
     else { /* ne_discard_response returned non-0 */
       /* close request, if it returns NE_RETRY, retry ... */
       // TBD: is it correct to call ne_end_request if  
ne_discard_response failed?
       rc = rq != NULL ? ne_end_request(rq) : NE_ERROR;

       if (rc != NE_OK && rc != NE_RETRY) {
         const char *nes = ne_get_error((ne_session *)this- 
 >ConnectionHandle());
         if (nes != NULL) { // copy, it might go away on con-close!
           assert(lastNeonError == NULL);
           lastNeonError = Z1String::New(nes);
         }
       }

       this->_CloseConnection(); // better be sure, close it
     }

     if (this->log != NULL) {
       if (rc == NE_RETRY) { // eg this is entered on 401
         this->log->PrintF("%s: got a NE_RETRY %i ...",
           "Z1HttpTransaction::Run", statCode);
       }
       else if (rc == NE_AUTH) {
         // we should never enter this?
         this->log->PrintF("%s: got a NE_AUTH %i: %s",
           "Z1HttpTransaction::Run", statCode,  
Z1StringUTF8(lastNeonError));
       }
       else if (rc != NE_OK) {
         this->log->PrintF("%s: got a error code %i, stat %i: %s",
           "Z1HttpTransaction::Run", rc, statCode,  
Z1StringUTF8(lastNeonError));
       }
     }

     /* hack to workaround Neon digest bug (exits if the nounce  
expired!) */
     if (rc == NE_AUTH && digestRetryCount < 3 && this->response !=  
NULL) {
       const wchar_t *wwwAuthenticate =
         Z1StringWide(this->response- 
 >StringHeaderForKey(Z1HttpHeader_WwwAuthenticate));

       wwwAuthenticate = z1_wcs_strstr(wwwAuthenticate, -1,
         "digest", true /* skipsp */, true /* ig case */);
       wwwAuthenticate = z1_wcs_strstr(wwwAuthenticate, -1,
         "stale", true /* skipsp */, true /* ig case */);
       if (wwwAuthenticate != NULL) {
         digestRetryCount++;
         rc = NE_RETRY;
         if (this->log != NULL) {
           this->log->PrintF("%s: WARN retry after stale digest nounce",
              "Z1HttpTransaction::Run");
         }
       }

       this->_CloseConnection(); // better be sure, close it
     }
   }
   while (rc == NE_RETRY);

   if (rc != NE_OK && this->response != NULL) {
     this->response->SetErrorCode(rc);

     if (lastNeonError != NULL)
       this->response->SetHeaderForKey(lastNeonError, L"X-NEON-ERROR");

     if (rc == NE_ERROR || rc == NE_LOOKUP) {
       this->response->SetContentAsString
         (ne_get_error((ne_session *)this->ConnectionHandle()));
     }
   }

   if (this->log != NULL && this->response != NULL) {
     this->log->PrintF("%s: Received response %i (read=%d)",
       "Z1HttpTransaction::Run", this->response->Status(), this- 
 >readCount);
     this->response->Dump(this->log);
     if (this->okOut != NULL) {
       this->log->PrintF("%s: [the output was streamed]",
         "Z1HttpTransaction::Run");
     }
   }

   /* teardown */

   if (this->decompress != NULL) {
     ne_decompress_destroy((ne_decompress *)this->decompress);
     this->decompress = NULL;
   }
   if (this->handle != NULL) {
     ne_request_destroy((ne_request *)this->handle);
     this->handle = NULL;
   }
---snap---





More information about the neon mailing list