3
\$\begingroup\$

I am using libxml library to parse the xml file into my code and then assign each element node to the corresponding linked list node. I think the code runs just fine,I hope, as I am still new to this and would like your review.

Is there anything that I may be doing wrong/dangerous approach?

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <libxml/parser.h>


struct node {
        char *polname;
        char *devname;
        char *status;
        char *srczone;
        char *dstzone;
        char *srcaddr;
        char *dstaddr;
        char *srcuser;
        char *app;
        char *service;
        char *urlcategory;
        char *action;
        char *vulnerability;


        struct node *next;
            };
        struct node *head = NULL;

void insert(char *a, char *b, char *c, char *d, char *e, char *f, char*g, char *h, char *i, char *j, char *k, char *l, char *m){
        struct node *temp;
        temp = (struct node*)malloc(sizeof(struct node));
        temp->polname = a;
        temp->devname = b;
        temp->status = c;
        temp->srczone = d;
        temp->dstzone = e;
        temp->srcaddr = f;
        temp->dstaddr = g;
        temp->srcuser = h;
        temp->app = i;
        temp->service = j;
        temp->urlcategory = k;
        temp->action = l;
        temp->vulnerability = m;
        temp->next = head;
        head= temp;


}    

void traverse_dom_trees(xmlNode * a_node)
{
    xmlNode *cur_node = NULL;

    for (cur_node = a_node; cur_node; cur_node = cur_node->next)
    {
                char *pol, *dev, *stat, *srcz, *dstz, *srca, *dsta, *srcu, *app, *ser, *url, *act, *vul;
                if(strcmp((const char *)cur_node->name,(const char *)"policyName") == 0){
                 xmlChar *a = xmlNodeGetContent(cur_node);
                        printf("printing a: %s\n", a);
                        pol = strdup(a);
                        printf("printing pol: %s\n", pol);
                        }
             else if(strcmp((const char *)cur_node->name,(const char *)"deviceName") == 0)  {
                    xmlChar *b = xmlNodeGetContent(cur_node);
                        printf("printing b: %s\n", b);
                        dev = strdup(b);
                        printf("printing dev: %s\n", dev);
                        }
            else if(strcmp((const char *)cur_node->name,(const char *)"status") == 0){
                    xmlChar *c = xmlNodeGetContent(cur_node);
                        printf("printing c: %s\n", c);
                        stat = strdup(c);
                        printf("printing stat: %s\n", stat);
                        }
            else if(strcmp((const char *)cur_node->name,(const char *)"srcZone") == 0){
                    xmlChar *d = xmlNodeGetContent(cur_node);
                        printf("printing d: %s\n", d);
                        srcz = strdup(d);
                        printf("printing srcz: %s\n", srcz);
                        }
            else if(strcmp((const char *)cur_node->name,(const char *)"dstZone") == 0){
                    xmlChar *e = xmlNodeGetContent(cur_node);
                        printf("printing e: %s\n", e);
                        dstz = strdup(e);
                        printf("printing dstz: %s\n", dstz);
 }
            else if(strcmp((const char *)cur_node->name,(const char *)"srcAddr") == 0){
                        xmlChar *f = xmlNodeGetContent(cur_node);
                        printf("printing f: %s\n", f);
                        srca = strdup(f);
                        printf("printing srca: %s\n", srca);
                        }
                else if(strcmp((const char *)cur_node->name,(const char *)"dstAddr") == 0){
                        xmlChar *g = xmlNodeGetContent(cur_node);
                        printf("printing g: %s\n", g);
                        dsta = strdup(g);
                        printf("printing dsta: %s\n", dsta);
                        }
                else if(strcmp((const char *)cur_node->name,(const char *)"srcUser") == 0){
                        xmlChar *h = xmlNodeGetContent(cur_node);
                        printf("printing h: %s\n", h);
                        srcu = strdup(h);
                        printf("printing srcu: %s\n", srcu);
                        }
                else if(strcmp((const char *)cur_node->name,(const char *)"application") == 0){
                       xmlChar *i = xmlNodeGetContent(cur_node);
                        printf("printing i: %s\n", i);
                        app = strdup(i);
                        printf("printing app: %s\n", app);
                        }
                else if(strcmp((const char *)cur_node->name,(const char *)"service") == 0){
                       xmlChar *j = xmlNodeGetContent(cur_node);
                        printf("printing j: %s\n", j);
                        ser = strdup(j);
                        printf("printing ser: %s\n", ser);
                        }
                else if(strcmp((const char *)cur_node->name,(const char *)"urlCategory") == 0){
                       xmlChar *k = xmlNodeGetContent(cur_node);
                        printf("printing k: %s\n", k);
                        url = strdup(k);
                        printf("printing url: %s\n", url);
                        }
                else if(strcmp((const char *)cur_node->name,(const char *)"action") == 0){
                       xmlChar *l = xmlNodeGetContent(cur_node);
                         printf("printing l: %s\n", l);
                        act = strdup(l);
                        printf("printing act: %s\n", act);
                        }
                else if(strcmp((const char *)cur_node->name,(const char *)"vulnerability") == 0){
                        xmlChar *m = xmlNodeGetContent(cur_node);
                         printf("printing m: %s\n", m);
                        vul = strdup(m);
 printf("printing vul: %s\n", vul);
                        }

          insert (pol, dev, stat, srcz, dstz, srca, dsta, srcu, app, ser, url, act, vul );
   //   print();

        traverse_dom_trees(cur_node->children);
    }
}
int main(int argc, char **argv)
{
    xmlDocPtr doc;
    xmlNode *roo_element = NULL;

    if (argc != 2)
    {
        printf("\nInvalid argument\n");
        return(1);
    }

    doc = xmlReadFile(argv[1], NULL, XML_PARSE_NOBLANKS | XML_PARSE_NOERROR | XML_PARSE_NOWARNING | XML_PARSE_NONET);
    if (doc == NULL)
    {
        fprintf(stderr, "Document not parsed successfully.\n");
        return 0;
    }

    roo_element = xmlDocGetRootElement(doc);

    if (roo_element == NULL)
 {
        fprintf(stderr, "empty document\n");
        xmlFreeDoc(doc);
        return 0;
    }

    printf("Root Node is %s\n", roo_element->name);
    traverse_dom_trees(roo_element);

    xmlFreeDoc(doc);       // free document
    xmlCleanupParser();    // Free globals
    return 0;
}

Since everybody asked me to provide the input file, so here it is... Sorry for the delay.

File.xml

<policyList>
        <policySecurity>
                <policyName>AutoAdd</policyName>
                <deviceName>PA-722</deviceName>
                <status>ACTIVE</status>
                <srcZone>any</srcZone>
                <dstZone>any</dstZone>
                <srcAddr>any</srcAddr>
                <dstAddr>5.5.5.4</dstAddr>
                <srcUser>any</srcUser>
                <application>any</application>
                <service>any</service>
                <urlCategory>any</urlCategory>
                <action>deny</action>
        </policySecurity>
        <policySecurity>
                <policyName>Test-1</policyName>
                <deviceName>PA-710</deviceName>
                <status>ACTIVE</status>
                <srcZone>any</srcZone>
                <dstZone>any</dstZone>
                <srcAddr>192.168.1.23</srcAddr>
                <dstAddr>8.8.8.8</dstAddr>
                <srcUser>vivek</srcUser>
                <application>any</application>
                <service>any</service>
                <urlCategory>any</urlCategory>
                <action>deny</action>
        </policySecurity>
        <policyVulnerability>
                <policyName>Test-1</policyName>
                <deviceName>PA-710</deviceName>
                <status>ACTIVE</status>
                <srcZone>any</srcZone>
                <dstZone>any</dstZone>
                <srcAddr>192.168.1.23</srcAddr>
                <dstAddr>8.8.8.8</dstAddr>
                <srcUser>vivek</srcUser>
                <application>any</application>
                <service>any</service>
                <urlCategory>any</urlCategory>
                <vulnerability>VP-33567</vulnerability>
                <action>deny</action>
        </policyVulnerability>
</policyList>
\$\endgroup\$
5
  • \$\begingroup\$ Code review posts should be working code \$\endgroup\$ Commented Dec 6, 2017 at 15:05
  • \$\begingroup\$ If your code doesn't work, post it on stackoverflow.com \$\endgroup\$ Commented Dec 6, 2017 at 22:53
  • \$\begingroup\$ @Snowbody It sure does work, sir. It just doesn't do a single part, i.e printing the linked list which is because of some internal conflict. I wanted to get the rest of the code reviewed... \$\endgroup\$ Commented Dec 7, 2017 at 5:25
  • \$\begingroup\$ Then you need to edit the post to not put so much emphasis on the bugs in the code. Submit working code and explain what it is doing. Maybe someone will spot the bugs as a bonus. \$\endgroup\$ Commented Dec 7, 2017 at 14:19
  • 2
    \$\begingroup\$ It's probably a good idea to include a (small) input file in the question too - that will make it easier for reviewers to understand, replicate and experiment with your code. \$\endgroup\$ Commented Dec 8, 2017 at 13:20

1 Answer 1

Reset to default
3
\$\begingroup\$

Consistency, Spelling and Variable Names

The code is really inconsistently spaced and bracketed, this makes it a much harder to read than necessary. This goes for us reviewers but it also goes for you. I know that people want to be concise in c-code but there really is no reason to leave the 't' off root_element for example. Also in traverse_dom_trees or your structure yes that's a handful to write but readable names make things a lot more maintainable.

node is probably a bad name for your structure, as there are already nodes in the xml tree, i'd prefer a name that describes what the data is. This helps to make things clearer when you move data from a node to your data structure, but also later in your program when you are dealing with the data in your structure but it's still called a node.

malloc can be used with variables

ptr = malloc(sizeof *ptr) is a valid expression, it's safer to use when the type of the variable may change (this means in most cases). sizeof also doesnt need parenthesis when the argument is an expression rather than a type.

Leaking memory

strdup,insert, and xmlNodeGetContent allocate memory this memory is never released anywhere, the resulting pointers should be deallocated somewhere before the program exits. As xmlNodeGetContent already returns allocated memory, there is no need to strdup the returned string. You should add functions to free the string in struct_node, and to free the list you are building.

No check for NULL return value with strdup and malloc

Both strdup and malloc may return NULL, depending what you want to do with the result you need to check for that before you proceed. Especially in insert as you are dereferencing the result. Dereferencing a pointer that is == NULL will cause an error.

Literal strings are of type const char*

There is no need to cast a literal string to const char *.

issues with insert

By passing each of the members of struct node you make the insert function dependent on that struct, in the long run that is a bad habit. If you need all the members of that struct, you might as well pass the struct into the function. This also separates populating your struct from inserting into the list. The same is true for the body, by copying each member by name, the list in there becomes dependent on being manually maintained. As mentioned above malloc is not checked. Here is a possible implementation for your insert.

int list_insert_front(struct node* new_node) {
    struct node *temp;
    temp = malloc(sizeof *temp);
    if (temp && new_node) {
        memcpy(temp, new_node, sizeof(struct node));
        temp->next = head;
        head = temp;
        return 1;
    }
    return 0;
}

As you can see this takes a struct_node and just copies it into the list.

Refactoring traverse_dom_trees

This does a lot of extra work that doesn't need to be done, but by using struct node as storage for the data read, the complexity can be reduced.

struct node current = {};
for (cur_node = a_node; cur_node; cur_node = cur_node->next) {
    const char* node_name = cur_node->name;
    if (strcmp(node_name, "policyName") == 0) {
        current.polname = xmlNodeGetContent(cur_node)
    }
    else if ...
}

if (!valid(&current) || !list_insert_front(&current) ) {
    // Free the strings from xmlNodeGetContent
    free_node_data(&current);  
}

You already have a place where all the members of your struct are declared, there is no need to introduce, a variable for each member of the struct. This also lets you pass it to the insert function in a more compact way.

With regard to bugs

Without the xml it's hard to give you more information. For example there is no way for us to tell you whether the what you are parsing matches the xml that you are reading, you seem to assume that all the information is flat at the top without any grouping nodes, i find that unlikely but that doesn't mean it's incorrect.

Edit 1

After looking at the xml file, i would say you are iterating in the wrong level of the xml file. policyList is the root node, it's children are policySecurity nodes, with policyName being the first child of that node, and the other entries being siblings of the. You call traverse_dom_tree with the root node and then look at its siblings, i would expect there are none. root->children->children should get you to the first policyName element, of the first policySecurity

\$\endgroup\$
4
  • \$\begingroup\$ Sir, I get most of your fixes, which you suggested, but am unable to understand why strdup was a bad choice to use here. Is it because it is leading to the memory leak or because it could be done in a much simpler way which you suggested? Moreover, I have added the file.xml which was required in order to provide the sample input I am taking. \$\endgroup\$ Commented Dec 11, 2017 at 6:34
  • \$\begingroup\$ Also, now when I try compiling the code, it returns me the following: collect2: error: ld returned 1 exit status \$\endgroup\$ Commented Dec 11, 2017 at 8:16
  • \$\begingroup\$ Using strdup is bad in both accounts, the way you are doing was leaking twice, for strdup and for xmlNodeGetContent and it was doing unecessary work. The string had already been copied therefore there is no need to copy it again. \$\endgroup\$ Commented Dec 11, 2017 at 13:09
  • \$\begingroup\$ Thanks for that, and I get your point about getting it copied twice. Moreover, the parser I have used finds and gets the next child node for me that is policy name and its respective child nodes, so no issues with that I suppose... \$\endgroup\$ Commented Dec 12, 2017 at 5:49

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.