Couple more bugs. Some with fixes included.

[Suresh Sundriyal-2]

Hi Klaus,

I saw the '(Final?)' in the 2.2.0 tags branch, so I'm guessing you're
unsure about the status too. :) I apologize for these last minute
patches.

Well, I was going through the cim_data_stubs.c file and a couple of
things jumped out. Actually quite a few, so I decided to make one huge
changeset and send it along. However, it just got too big to comfortably
test for regressions and due to the lack of time I decided to fix the
things that I could easily fix and delay the rest for the weekend.

I'll try to explain the patch that I am sending along and what else I
think feels wrong in that particular file. If someone else can manage to
fix the rest of the problems before I can get to them, all the better. :)

Explanation for the patch:
==========================

1. Possible Segfault:

In method CimResource_Init(), there should be a check for allocation
failure right after the following line:

CimClientInfo *cimclient= (CimClientInfo*)u_zalloc(sizeof(CimClientInfo));

Considering we dereference cimclient->cc a couple of lines later, if
there is an allocation failure, this will lead to a segfault. The
included patch has a fix for this.

2. Memory Leak:

Now considering the case where the allocation has succeeded, if
initialization of cimclient->cc fails, we return with a NULL without
cleaning up the space allocated for cimclient.  This would definitely be
a memory leak. The attached patch has a fix for this as well.

3. Another Segfault:

If you look at CimResource_Delete_EP, we initialize cimclient only if
msg is not NULL and there is no else clause. This means if we get a case
where msg is NULL, cimclient is not initialized and passed as NULL to
verify_class_namespace, which does some funky stuff with it which is
guaranteed to segfault if we hit this corner case.

CimResource_Delete_EP is not the only culprit here, most of the endpoint
callback methods in this file suffer from this affliction.

The patch adds code to verify_class_namespace that checks whether the
passed in pointer is null or not and returns '0' if it is, which seems
to mitigate some of the problems associated with the enpoint callback
methods(more on that later).

4. Even more segfaults:

In verify_class_namespace, even if the *client is initialized, looking
at CimResource_Init, it seems that client->resource_uri,
client->requested_class and client->method can still be NULL. We pass
these potentially NULL pointers to strstr() and strcmp() which will
again cause a segfault. Not sure under what circumstances this will
happen but the code certainly has no provision to accommodate these
circumstances.

Now we come to the stuff that I haven't gotten to yet:
======================================================

1. If you look at most of the endpoint methods in cim_data_stubs.c, you
will see code similiar to:

  if (msg) {
     cimclient = CimResource_Init(cntx,
                             msg->auth_data.username,
                             msg->auth_data.password );
             if (!cimclient) {
                     status.fault_code = WSA_ENDPOINT_UNAVAILABLE;
                     status.fault_detail_code = 0;
                     goto cleanup;
             }
         }

This code has two problems as far as I can tell. One if msg is NULL, it
should return an error and not continue further, and hence should have
an else clause (I could be wrong but seems that way to me).

Second, this allows for cimclient to be null in case msg is null and the
code continues further. Even though the fix in verify_class_namespace in
the attached patch takes care of this condition, I feel the code should
be cleaned up to do the right thing.

The correct way to write this would be :


if(msg){
    cimclient = CimResource_Init(cntx,
                            msg->auth_data.username,
                             msg->auth_data.password );


}
else{
    status.fault_code = WSE_INVALID_MESSAGE;
    status.fault_detail_code=0;
    goto cleanup;
    //or some other appropriate error message.
}
if(!cimclient){
        status.fault_code = WSA_ENDPOINT_UNAVAILABLE;
         status.fault_detail_code = 0;
         goto cleanup;
}

There are slight variations here and there but I believe most of the
methods suffer from this problem.

2. There are a few cases where within the the endpoint methods, possible
null pointers are passed to strstr or strcmp, these need to be fixed.


I hope all that makes sense. This is just me eyeballing the code and
commenting on what looks wrong. Maybe those codepaths will never be
exercised, since I haven't see crashes related to these methods.

--
Regards,
Suresh
[mem-leak-etc.diff]

---

Index: src/plugins/cim/cim_data_stubs.c
===================================================================
--- src/plugins/cim/cim_data_stubs.c (revision 3252)
+++ src/plugins/cim/cim_data_stubs.c (working copy)
@@ -59,6 +59,50 @@
 #include "cim_data.h"
 
 
+static void
+CimResource_destroy(CimClientInfo *cimclient)
+{
+ if (!cimclient)
+ return;
+ if (cimclient->resource_uri)
+ u_free(cimclient->resource_uri);
+ if (cimclient->method)
+ u_free(cimclient->method);
+ if (cimclient->requested_class)
+ u_free(cimclient->requested_class);
+ if (cimclient->method_args) {
+                /* FIXME: this should use a 'free' function from hash.c with a per-node 'free' callback */
+ hscan_t hs;
+                hnode_t *hn;
+                selector_entry *sentry = NULL;
+                hash_t * args = cimclient->method_args;
+                hash_scan_begin(&hs, args);
+                while ((hn = hash_scan_next(&hs)))
+                {
+                        sentry = (selector_entry*)hnode_get(hn);
+                        if(NULL != sentry)
+                         u_free(sentry);
+                }
+
+ hash_free(cimclient->method_args);
+ }
+ if (cimclient->selectors) {
+ hash_free(cimclient->selectors);
+ debug("selectors destroyed");
+ }
+ if (cimclient->username)
+ u_free(cimclient->username);
+ if (cimclient->password)
+ u_free(cimclient->password);
+ cim_release_client(cimclient);
+ u_free(cimclient);
+ debug("cimclient destroyed");
+ return;
+}
+
+
+
+
 static CimClientInfo*
 CimResource_Init(WsContextH cntx, char *username, char *password)
 {
@@ -66,6 +110,9 @@
  char *resource_uri = NULL;
  char *show_extensions;
  CimClientInfo *cimclient= (CimClientInfo *)u_zalloc(sizeof(CimClientInfo));
+ if(!cimclient){
+ return NULL;
+ }
  WsmanStatus status;
 
  wsman_status_init(&status);
@@ -78,6 +125,7 @@
  get_cim_port(), username, password , get_cim_client_frontend(), &status);
 
  if (!cimclient->cc) {
+ CimResource_destroy(cimclient);
  return NULL;
  }
 
@@ -120,71 +168,31 @@
  return cimclient;
 }
 
-static void
-CimResource_destroy(CimClientInfo *cimclient)
-{
- if (!cimclient)
- return;
- if (cimclient->resource_uri)
- u_free(cimclient->resource_uri);
- if (cimclient->method)
- u_free(cimclient->method);
- if (cimclient->requested_class)
- u_free(cimclient->requested_class);
- if (cimclient->method_args) {
-                /* FIXME: this should use a 'free' function from hash.c with a per-node 'free' callback */
- hscan_t hs;
-                hnode_t *hn;
-                selector_entry *sentry = NULL;
-                hash_t * args = cimclient->method_args;
-                hash_scan_begin(&hs, args);
-                while ((hn = hash_scan_next(&hs)))
-                {
-                        sentry = (selector_entry*)hnode_get(hn);
-                        if(NULL != sentry)
-                         u_free(sentry);
-                }
-
- hash_free(cimclient->method_args);
- }
- if (cimclient->selectors) {
- hash_free(cimclient->selectors);
- debug("selectors destroyed");
- }
- if (cimclient->username)
- u_free(cimclient->username);
- if (cimclient->password)
- u_free(cimclient->password);
- cim_release_client(cimclient);
- u_free(cimclient);
- debug("cimclient destroyed");
- return;
-}
-
-
-
-
 static int
 verify_class_namespace(CimClientInfo *client)
 {
  hscan_t hs;
  hnode_t *hn;
  int rv = 0;
- if ( strcmp( client->resource_uri, CIM_ALL_AVAILABLE_CLASSES ) ==0 ) {
+ if(!client){
+ return 0;
+ }
+ if (client->resource_uri && (strcmp( client->resource_uri, CIM_ALL_AVAILABLE_CLASSES ) ==0) ) {
  return 1;
  }
-        if ( strstr( client->resource_uri, XML_NS_CIM_INTRINSIC ) != NULL ) {
+        if ( client->resource_uri && (strstr( client->resource_uri, XML_NS_CIM_INTRINSIC ) != NULL )) {
         return 1;
  }
   
         /* Ok if class contains CIM, uri contains XML_NS_CIM_CLASS, and method is not 'Create' */
- if ( (strstr(client->requested_class, "CIM") != NULL )
+ if (client->requested_class && client->resource_uri && client->method
+            && (strstr(client->requested_class, "CIM") != NULL )
     && (strstr(client->resource_uri , XML_NS_CIM_CLASS) != NULL )
     && (strcmp(client->method, TRANSFER_CREATE) != 0)) {
  return 1;
  }
 
- if (client->requested_class && client->namespaces) {
+ if (client->requested_class && client->namespaces && client->resource_uri) {
  hash_scan_begin(&hs, client->namespaces);
  while ((hn = hash_scan_next(&hs))) {
  if ( ( strstr(client->requested_class,

---

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Klaus Kaempf]

* Suresh Sundriyal <ssundriy@...> [Sep 11. 2009 07:32]:
> Hi Klaus,
>
> I saw the '(Final?)' in the 2.2.0 tags branch, so I'm guessing you're
> unsure about the status too. :)

:-)
Yes, I am unsure. And its just as I expected: People only start real
tests when something 'final' is out :-)

> I apologize for these last minute patches.

NP, thanks for your testing efforts !

I'll look through your patches later today.

Klaus
---
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Klaus Kaempf]

Suresh,

* Suresh Sundriyal <ssundriy@...> [Sep 11. 2009 07:32]:
>
> I hope all that makes sense. This is just me eyeballing the code and
> commenting on what looks wrong. Maybe those codepaths will never be
> exercised, since I haven't see crashes related to these methods.

this all makes a lot of sense, thanks again for your review !

I just applied your fix as svn rev 3253.

Regarding the error returned if 'msg' is NULL, WSE_INVALID_MESSAGE is
probably not appropriate because the 'WSE' prefix indicates a
WS-Enumeration error.

However, I cannot find a better error response in wsman-faults.h :-(

Klaus
---
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Klaus Kaempf]

* Suresh Sundriyal <ssundriy@...> [Sep 11. 2009 07:33]:

>
> The correct way to write this would be :
>
>
> if(msg){
>     cimclient = CimResource_Init(cntx,
>            msg->auth_data.username,
>                              msg->auth_data.password );
>
>
> }
> else{
>     status.fault_code = WSE_INVALID_MESSAGE;
>     status.fault_detail_code=0;
>     goto cleanup;
>     //or some other appropriate error message.
> }
> if(!cimclient){
> status.fault_code = WSA_ENDPOINT_UNAVAILABLE;
>          status.fault_detail_code = 0;
>          goto cleanup;
> }
>
> There are slight variations here and there but I believe most of the
> methods suffer from this problem.

svn rev 3254 now contains fixes for these problems.

Klaus
---
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Suresh Sundriyal-2]

Hi Klaus,

I took a brief glance at rev 3254. The changes look good. Thanks for taking care of that. :)

I'm just going by pattern recognition here, since I don't have access to compilers or my Openwsman setup here but I am sending
across a couple more additions. I haven't compiled the code or done any testing (Beware).  All I did was take a look at the code and pick out stuff that looked odd.
Most of it has to do with some places where you don't set the retval before returning an error, whereas right around the same code block, you do set the retval
before returning another error. In another place, you construct the error document before returning one error and don't for the error immidiately before. I know I'm
sounding really vague at this point but I'm pretty much half asleep.

If all this sounds complete gibberish, just let it be and I promise to get back with a much more coherent explanation tomorrow. (It could very well turn out that
lack of sleep is making me paranoid. :))

--
Regards,
Suresh
________________________________________
From: Klaus Kaempf [kkaempf@...]
Sent: Friday, September 11, 2009 2:05 AM
To: Suresh Sundriyal
Cc: openwsman-devel@...
Subject: Re: [Openwsman-devel] Couple more bugs. Some with fixes included.

* Suresh Sundriyal <ssundriy@...> [Sep 11. 2009 07:33]:

>
> The correct way to write this would be :
>
>
> if(msg){
>     cimclient = CimResource_Init(cntx,
>                           msg->auth_data.username,
>                              msg->auth_data.password );
>
>
> }
> else{
>     status.fault_code = WSE_INVALID_MESSAGE;
>     status.fault_detail_code=0;
>     goto cleanup;
>     //or some other appropriate error message.
> }
> if(!cimclient){
>       status.fault_code = WSA_ENDPOINT_UNAVAILABLE;
>          status.fault_detail_code = 0;
>          goto cleanup;
> }
>
> There are slight variations here and there but I believe most of the
> methods suffer from this problem.

svn rev 3254 now contains fixes for these problems.

Klaus
---
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)


[additions.diff]

---

Index: plugins/cim/cim_data_stubs.c
===================================================================
--- plugins/cim/cim_data_stubs.c (revision 3254)
+++ plugins/cim/cim_data_stubs.c (working copy)
@@ -431,6 +431,7 @@
  if (!enumInfo) {
  status->fault_code = WSMAN_SCHEMA_VALIDATION_ERROR;
  status->fault_detail_code=0;
+ retval = 1;
  goto cleanup;
  }
 
@@ -521,6 +522,8 @@
  if (!enumInfo) {
  status->fault_code = WSMAN_SCHEMA_VALIDATION_ERROR;
  status->fault_detail_code=0;
+ doc = wsman_generate_fault( cntx->indoc, status->fault_code,
+ status->fault_detail_code, NULL);
  goto cleanup;
  }
 
@@ -748,6 +751,7 @@
  if (!subsInfo) {
  status->fault_code = WSMAN_SCHEMA_VALIDATION_ERROR;
  status->fault_detail_code=0;
+ retval = 1;
  goto cleanup;
  }
  cimclient = CimResource_Init(cntx,

---

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel

[Suresh Sundriyal-2]

Hi Klaus,

I'm sending along another patch which adds to rev 3254. Since you
haven't applied the previous patch that I sent out last night, this one
is a superset of the previous one (additions.diff). Here's an
explanation of what the patch does.

1. In CimResource_Delete_EP, the last 'else if' conditional seems
pointless, considering if the code has reached this point then the
'cleanup' section is definitely going to be invoked, which does the
exact same thing. The code I'm talking about is this:

         else if (status.fault_code != 0) {
                 ws_xml_destroy_doc(doc);
                 doc = wsman_generate_fault(in_doc, status.fault_code,
                                 status.fault_detail_code, NULL);
         }
debug("here");
cleanup:
         if (wsman_check_status(&status) != 0) {
                 ws_xml_destroy_doc(doc);
                 doc = wsman_generate_fault( soap_get_op_doc(op, 1),
                                 status.fault_code,
status.fault_detail_code, NULL);
         }


2. In CimResource_Get_EP, in the following section:


        if (!verify_class_namespace(cimclient) ) {
                 status.fault_code = WSA_DESTINATION_UNREACHABLE;
            status.fault_detail_code = WSMAN_DETAIL_INVALID_RESOURCEURI;
                 doc = wsman_generate_fault( in_doc, status.fault_code,
                                 status.fault_detail_code, NULL);
         }
the 'doc=wsman_generate_fault' section seems redundant, since the
cleanup code will do it, considering we have set the fault_code.

3. The rest of it is just adding some stuff to error correction that you
missed out, like setting retval or invoking wsman_generate_fault where
it should be generated.

If you need more explanation on the last bit just let me know.

--
Regards,
Suresh


Suresh Sundriyal wrote:

> Hi Klaus,
>
> I took a brief glance at rev 3254. The changes look good. Thanks for taking care of that. :)
>
> I'm just going by pattern recognition here, since I don't have access to compilers or my Openwsman setup here but I am sending
> across a couple more additions. I haven't compiled the code or done any testing (Beware).  All I did was take a look at the code and pick out stuff that looked odd.
> Most of it has to do with some places where you don't set the retval before returning an error, whereas right around the same code block, you do set the retval
> before returning another error. In another place, you construct the error document before returning one error and don't for the error immidiately before. I know I'm
> sounding really vague at this point but I'm pretty much half asleep.
>
> If all this sounds complete gibberish, just let it be and I promise to get back with a much more coherent explanation tomorrow. (It could very well turn out that
> lack of sleep is making me paranoid. :))
>
> --
> Regards,
> Suresh
> ________________________________________
> From: Klaus Kaempf [kkaempf@...]
> Sent: Friday, September 11, 2009 2:05 AM
> To: Suresh Sundriyal
> Cc: openwsman-devel@...
> Subject: Re: [Openwsman-devel] Couple more bugs. Some with fixes included.
>
> * Suresh Sundriyal <ssundriy@...> [Sep 11. 2009 07:33]:
>> The correct way to write this would be :
>>
>>
>> if(msg){
>>     cimclient = CimResource_Init(cntx,
>>                           msg->auth_data.username,
>>                              msg->auth_data.password );
>>
>>
>> }
>> else{
>>     status.fault_code = WSE_INVALID_MESSAGE;
>>     status.fault_detail_code=0;
>>     goto cleanup;
>>     //or some other appropriate error message.
>> }
>> if(!cimclient){
>>       status.fault_code = WSA_ENDPOINT_UNAVAILABLE;
>>          status.fault_detail_code = 0;
>>          goto cleanup;
>> }
>>
>> There are slight variations here and there but I believe most of the
>> methods suffer from this problem.
>
> svn rev 3254 now contains fixes for these problems.
>
> Klaus
> ---
> SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)
>
>

Index: src/plugins/cim/cim_data_stubs.c
===================================================================
--- src/plugins/cim/cim_data_stubs.c (revision 3254)
+++ src/plugins/cim/cim_data_stubs.c (working copy)
@@ -253,11 +253,6 @@
  cim_delete_instance(cimclient, &status);
  }
  }
- else if (status.fault_code != 0) {
- ws_xml_destroy_doc(doc);
- doc = wsman_generate_fault(in_doc, status.fault_code,
- status.fault_detail_code, NULL);
- }
 debug("here");
 cleanup:
  if (wsman_check_status(&status) != 0) {
@@ -308,8 +303,6 @@
  if (!verify_class_namespace(cimclient) ) {
  status.fault_code = WSA_DESTINATION_UNREACHABLE;
  status.fault_detail_code = WSMAN_DETAIL_INVALID_RESOURCEURI;
- doc = wsman_generate_fault( in_doc, status.fault_code,
- status.fault_detail_code, NULL);
  } else {
  if ( (doc = wsman_create_response_envelope( in_doc, NULL)) ) {
  WsXmlNodeH body = ws_xml_get_soap_body(doc);
@@ -364,6 +357,8 @@
  if (!msg) {
  status.fault_code = WSMAN_SCHEMA_VALIDATION_ERROR;
  status.fault_detail_code=0;
+ doc = wsman_generate_fault( in_doc, status.fault_code,
+                                status.fault_detail_code, status.fault_msg);
  goto cleanup;
  }
  cimclient = CimResource_Init(cntx, msg->auth_data.username,
@@ -371,6 +366,8 @@
  if (!cimclient) {
  status.fault_code = WSA_ENDPOINT_UNAVAILABLE;
  status.fault_detail_code = 0;
+ doc = wsman_generate_fault( in_doc, status.fault_code,
+                                status.fault_detail_code, status.fault_msg);
  goto cleanup;
  }
  if (action && cimclient->resource_uri &&
@@ -431,6 +428,7 @@
  if (!enumInfo) {
  status->fault_code = WSMAN_SCHEMA_VALIDATION_ERROR;
  status->fault_detail_code=0;
+ retval=1;
  goto cleanup;
  }
 
@@ -521,6 +519,8 @@
  if (!enumInfo) {
  status->fault_code = WSMAN_SCHEMA_VALIDATION_ERROR;
  status->fault_detail_code=0;
+ doc = wsman_generate_fault( cntx->indoc, status->fault_code,
+                        status->fault_detail_code, NULL);
  goto cleanup;
  }
 
@@ -748,6 +748,7 @@
  if (!subsInfo) {
  status->fault_code = WSMAN_SCHEMA_VALIDATION_ERROR;
  status->fault_detail_code=0;
+ retval=1;
  goto cleanup;
  }
  cimclient = CimResource_Init(cntx,

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Openwsman-devel mailing list
Openwsman-devel@...
https://lists.sourceforge.net/lists/listinfo/openwsman-devel