No fclose operation after using file pointer

[XWwalker]

ppp/pppd/plugins/radius/config.c：In the rc_read_config function, it seems that in some cases there is no fclose(configfd) operation on the configfd file pointer

ppp/pppd/plugins/radius/config.c：188 line

int rc_read_config(char *filename)
{
    FILE *configfd;
    char buffer[512], *p;
    OPTION *option;
    int line, pos;

    if ((configfd = fopen(filename,"r")) == NULL)
    {
        error("rc_read_config: can't open %s: %m", filename);
        return (-1);
    }

    line = 0;
    while ((fgets(buffer, sizeof(buffer), configfd) != NULL))
    {
        line++;
        p = buffer;

        if ((*p == '\n') || (*p == '#') || (*p == '\0'))
            continue;

        p[strlen(p)-1] = '\0';

        if ((pos = strcspn(p, "\t ")) == 0) {
            error("%s: line %d: bogus format: %s", filename, line, p);
            return (-1);
        }

        p[pos] = '\0';

        if ((option = find_option(p, OT_ANY)) == NULL) {
            warn("%s: line %d: unrecognized keyword: %s", filename, line, p);
            continue;
        }

        if (option->status != ST_UNDEF) {
            error("%s: line %d: duplicate option line: %s", filename, line, p);
            return (-1);
        }

        p += pos+1;
        while (isspace(*p))
            p++;

        switch (option->type) {
            case OT_STR:
                 if (set_option_str(filename, line, option, p) < 0)
                    return (-1);
                break;
            case OT_INT:
                 if (set_option_int(filename, line, option, p) < 0)
                    return (-1);
                break;
            case OT_SRV:
                 if (set_option_srv(filename, line, option, p) < 0)
                    return (-1);
                break;
            case OT_AUO:
                 if (set_option_auo(filename, line, option, p) < 0)
                    return (-1);
                break;
            default:
                fatal("rc_read_config: impossible case branch!");
                abort();
        }
    }
    fclose(configfd);

    return test_config(filename);
}

[XWwalker]

if ((pos = strcspn(p, "\t ")) == 0) {
    error("%s: line %d: bogus format: %s", filename, line, p);
    return (-1);
}

For example, in this branch, fclose(configfd) is not executed before return -1, which may lead to a security risk

[Neustradamus]

@enaess: ^

[XWwalker]

In some exception branches before return -1, the flcose(configfd) operation is not executed, consider executing the flcose(configfd) operation before return -1.

Example： Execute the flcose(configfd) operation before return -1 to fix the problem.

    if ((pos = strcspn(p, "\t ")) == 0) {
        error("%s: line %d: bogus format: %s", filename, line, p);
+       flcose(configfd);
        return (-1);
    }

[enaess]

I got to double check, but the coverity checks flagged this issue. I made a PR that should cover closing the sockets and freeing the memory in this function

[XWwalker]

Thanks

[XWwalker]

@enaess

pppd/plugins/radius/clientid.c：A similar problem was found in this .c file， fclose(configfd) is not executed before return -1.

pppd/plugins/radius/clientid.c：Line42

int rc_read_mapfile(char *filename)
{
    char buffer[1024];
    FILE *mapfd;
    char *c, *name, *id, *q;
    struct map2id_s *p;
    int lnr = 0;

    if ((mapfd = fopen(filename,"r")) == NULL)
    {
        error("rc_read_mapfile: can't read %s: %s", filename, strerror(errno));
        return (-1);
    }

#define SKIP(p) while(*p && isspace(*p)) p++;

    while (fgets(buffer, sizeof(buffer), mapfd) != NULL)
    {
        lnr++;

        q = buffer;

        SKIP(q);

        if ((*q == '\n') || (*q == '#') || (*q == '\0'))
            continue;

        if (( c = strchr(q, ' ')) || (c = strchr(q,'\t'))) {

            *c = '\0'; c++;
            SKIP(c);

            name = q;
            id = c;

            if ((p = (struct map2id_s *)malloc(sizeof(*p))) == NULL) {
                novm("rc_read_mapfile");
                return (-1);
            }

            p->name = strdup(name);
            p->id = atoi(id);
            p->next = map2id_list;
            map2id_list = p;

        } else {

            error("rc_read_mapfile: malformed line in %s, line %d", filename, lnr);
            return (-1);

        }
    }

#undef SKIP

    fclose(mapfd);

    return 0;
}

[enaess]

@XWwalker What tools are you using to find these issues? Coverity did flag radius/config.c for two other reasons, but not this.

Should be rather trivial in nature, and if you are able to submit a PR for this; please go ahead. If you are not, then I maybe be able to do this a evening this week.

On a slightly different note, I am not a big fan of copying a version of freeradius into the code base. While it could serve as a backup, we should really be using libfreeradius (or whatever most recent project that provides that functionality). That stuff could be done with some autoconf detection. We are currently doing that with libatm...

[XWwalker]

Of course. I'll submit a PR later today. regarding tools, I'm not using tools like Coverity, I'm just recently interested in the ppp protocol and want to read the ppp related code. I will fix this problem later by submitting a PR on this. Thank you for your contributions to the community and your code, it has helped me a lot.

[XWwalker]

@enaess PR has been submitted, please review it.