Thread: invalid pointer

For starters, you need to format your code properly, especially where indentation is concerned. Generally, you only need one or two blank lines to separate parts of the code. Your indent style and spacing should also be consistent. For example:

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>


#define TRUE 0
#define FALSE -1
#define MAXERRORS 50  // Maximum Errors we record
#define ERRLEN 250


int debug = 1;


char *FilterChars(char *String, char *Filter)
{
    int a = 0, i = 0;
    char *Filtered = (char *)malloc(strlen(String) * sizeof(char));
    for (a = 0; String[a]; a++)
        if (!strchr(Filter, String[a]))
            Filtered[i++] = String[a];
    Filtered[i] = 0;
    return Filtered;
}


char *GetVirtual(char *command, char *configfilename, char *vendor)
{
    char cmd[50] = "", path[50] = "", buffer[80] = "";
    char *hvvend = malloc(50 * sizeof(char));
    char *tmppath = NULL;
    int syserr = 0;
    char *errmsg = malloc(ERRLEN * sizeof(char));
    FILE *fp = NULL;

    sprintf(cmd, "/usr/bin/lscpu > /tmp/lscpu");
    syserr = system(cmd);

    if (debug == 1)
        printf ("\nGetVirtual - cmd returned %d", syserr);

    if (syserr != 0)
    {
        sprintf(errmsg, "error executing lscpu- cannot fetch information regarding virtualisation. check config-file parameter lscpu");
        strcpy(vendor, "-");
    }

    if (syserr == 0)
    {
        sprintf(path, "/tmp/lscpu");
        fp = fopen(path, "r");
        while (!feof(fp))
        {
            fgets(buffer, 80, fp);
            if (strstr(buffer, "Hypervisor"))
            {
                strcpy(hvvend, buffer);
                strtok(hvvend, ":");
                vendor = strtok(NULL,":");
                vendor = FilterChars(vendor, " \n");
                if (strcmp(vendor, "VMware") == 0)
                    vendor = "VMWare";
                if (strcmp(vendor, "Microsoft") == 0)
                    vendor = "Hyper-V";
                if (debug == 1)
                    printf("\nHV-Vendor: %s", vendor);
                break;
            }
        }
        fclose(fp);
        sprintf(cmd, "rm -f /tmp/lscpu");
        system(cmd);
    }

    if (debug == 1)
    {
        printf("\nHV-Vendor: %s", vendor);
        printf("\nPointer Address: %p", vendor);
    }

    return vendor;
}


int main(void)
{
    char *hypervisor = malloc(8 * sizeof(char));

    hypervisor = GetVirtual("/usr/bin/lscpu", "/etc/collector.cfg", hypervisor);
    if (debug == 1)
        printf("\nHV Pointer Address: %p", hypervisor);
    printf("size of hv is %d", sizeof(hypervisor));
    free(hypervisor);
}

Originally Posted by hscharf

Can somebody please give me a hint, why I cannot free the char* hypervisor? - i get an "ivalid pointer" error when I try to free

The hypervisor actual argument in main corresponds to the vendor formal parameter in GetVirtual. In GetVirtual, we can see:

Code:

if (strcmp(vendor, "VMware") == 0)
    vendor = "VMWare";
if (strcmp(vendor, "Microsoft") == 0)
    vendor = "Hyper-V";

At first glance, this looks okay: after all, vendor is a copy of hypervisor, so although you are assigning a string literal (or rather, a pointer to the first element of a string literal), it is no big deal as vendor is after all a local variable. Then, we see:

Code:

return vendor;

followed by:

Code:

hypervisor = GetVirtual(/* etc */);

Hence, the value of hypervisor becomes a copy of the value of vendor, but vendor may now be a pointer to the first element of a string literal! Therefore, you would be unable to free(hypervisor).

There are a few possible solutions. One solution is to document that GetVirtual will return a string literal (or a null pointer in case of error), then you can do:

Code:

const char *GetVirtual(char *command, char *configfilename)
{
    const char *hypervisor = NULL;
    char vendor[8];

    /* ... */

    if (syserr == 0)
    {
        sprintf(path, "/tmp/lscpu");
        fp = fopen(path, "r");
        if (fp)
        {
            while (fgets(buffer, 80, fp))
            {
                if (strstr(buffer, "Hypervisor"))
                {
                    strcpy(hvvend, buffer);
                    strtok(hvvend, ":");
                    vendor = strtok(NULL,":");
                    vendor = FilterChars(vendor, " \n");
                    if (strcmp(vendor, "VMware") == 0)
                        hypervisor = "VMWare";
                    if (strcmp(vendor, "Microsoft") == 0)
                        hypervisor = "Hyper-V";
                    if (debug == 1)
                        printf("\nHV-Vendor: %s", hypervisor);
                    break;
                }
            }
            fclose(fp);
        }
        sprintf(cmd, "rm -f /tmp/lscpu");
        system(cmd);
    }

    /* ... */
    
    return hypervisor;
}

int main(void)
{
    char *hypervisor = GetVirtual("/usr/bin/lscpu", "/etc/collector.cfg");
    if (debug == 1)
        printf("\nHV Pointer Address: %p", hypervisor);
    printf("size of hv is %d", sizeof(hypervisor));
}

This way, you neither need to free vendor nor hypervisor, whether in GetVirtual or main.

Notice that instead of using feof, I used fgets to control the loop.

Of course, there are other ways, e.g., use strcpy instead of assignment.

Originally Posted by hscharf

What I don't understand right now... what happens to the pointer,
when I assign the string to the variable without strcpy (means: what is the main difference to this):

Basically, you end up with something like this:

Code:

int main(void)
{
    char *hypervisor = malloc(8 * sizeof(char));
    hypervisor = "VMWare";
    /* #1 */
    free(hypervisor);
}

as you can see, at the point #1, you pretty much have a memory leak because hypervisor no longer points to the dynamically allocated memory but instead it points to the first character of the string literal "VMWare". Worse still, you then attempt to free(hypervisor), but that must be wrong because it does not point to memory allocated via malloc (and friends).

Contrast with:

Code:

int main(void)
{
    char *hypervisor = malloc(8 * sizeof(char));
    strcpy(hypervisor, "VMWare");
    /* #1 */
    free(hypervisor);
}

Now, the dynamically allocated array contains a copy of the string "VMWare", and since hypervisor still points to the memory from malloc, you can free(hypervisor).