warnings, that asprintf() return code is not checked

[ftasnetamot]

I reported in #468 warnings, I see with newer compilers. Its mostly about the use of asprintf(). asprintf() has an implicit malloc() function and needs to be checked like malloc(), to make sure, that no invalid pointers are dangling around.

I have checked some of the locations, and it looks like, that some changes are very easy, others need rewriting of An easy example looks like:

  if (config_setting_lookup_string(setting, name, &str) == CONFIG_TRUE) {
        asprintf(value, "%s", str);
        return CONFIG_TRUE;
    } else {
        return CONFIG_FALSE;
    }

which could rewritten to:

  if ((config_setting_lookup_string(setting, name, &str) == CONFIG_TRUE) && (asprintf(value, "%s", str) != -1)){
       return CONFIG_TRUE;
   } else {
       return CONFIG_FALSE;
   }

Other locations are much more difficult:

static int c2s_parse_file(const char* filename, config_t* c, char**errmsg)
{
    /* Read config file */
    if (config_read_file(c, filename) == CONFIG_FALSE) {
        if (config_error_line(c) != 0) {
           asprintf(errmsg, "%s:%d:%s", 
                    filename,
                    config_error_line(c),
                    config_error_text(c));
           return 0;
        }
        asprintf(errmsg, "%s:%s", filename, config_error_text(c));
        return 0;
    }
    return 1;
}

Here is an error value of "0" returned, and exactly in this case the pointer char**errmsg will be used to report the error. A situation, which should be avoided, if the implicit malloc() of asprintf() failed to reserve the needed memory.

Locations, whith the unchecked asprintf() calls:

sslh-conf.c:229:9:
sslh-conf.c:2109:12:
sslh-conf.c:2115:9:
sslh-conf.c:1820:13:
sslh-conf.c:1763:17:
sslh-conf.c:313:9:
sslh-conf.c:356:9:
sslh-conf.c:2010:9:
sslh-conf.c:2020:9:
sslh-conf.c:2170:13:
sslh-conf.c:2172:13:
sslh-conf.c:2180:17:
sslh-conf.c:2183:17:
sslh-conf.c:2185:13:
sslh-conf.c:2126:9:
sslh-conf.c:2130:9:
sslh-conf.c:2134:9:
sslh-conf.c:2138:9:
sslh-conf.c:2142:9:
echosrv-conf.c:229:9:
echosrv-conf.c:1114:12:
echosrv-conf.c:1120:9:
echosrv-conf.c:825:13:
echosrv-conf.c:768:17:
echosrv-conf.c:313:9:
echosrv-conf.c:356:9:
echosrv-conf.c:1015:9:
echosrv-conf.c:1025:9:
echosrv-conf.c:1175:13:
echosrv-conf.c:1177:13:
echosrv-conf.c:1185:17:
echosrv-conf.c:1188:17:
echosrv-conf.c:1190:13:
echosrv-conf.c:1131:9:
echosrv-conf.c:1135:9:
echosrv-conf.c:1139:9:
echosrv-conf.c:1143:9:
echosrv-conf.c:1147:9:

What I have not checked yet, if all of the allocated memory from asprintf gets properly deallocated. When I cross-check the source, all malloc() i have seen so far, are checked.

[ftasnetamot]

I have done now a first check, how the asprintf() allocated memory structures are handled. Looks like, there is something to do.

I looked at the first occurence of asprintf(), where asprintf() is used in function _myconfig_setting_lookupstringcpy().

This is only used just once a little bit later in the same c-file as part of a table _lookup_fns_, where lookup functions are stored.

This table with the stored functions is used again in the same file, where we have to differentiate two cases:

• a return of CONFIG_TRUE, where the asprintf() result is stored in the presented pointer
• a return of CONFIG_FALSE, where a second call is made, after all dashes in the name are changed to underscore. In this second call there is no check of the response code, but there is now also the asprintf() result stored in the presented pointer

The above referenced function is used again in the same file, where as a result of a failing lookup another asprintf() is used, to create an _errmsg__.

Now I have two traces. Following the configuration path or having a look at the errmsg. I decided to the latter for my next step: So the question is, from where is read_block_setval() called Luckily its only called once from read_block_setval() and here we have our first memory leak:

  ....
        set = read_block_setval(target, cfg, desc, errmsg);
        if (!set && desc->mandatory) {
            asprintf(errmsg, "Mandatory option \"%s\" not found", desc->name);
            return 0;
        }
  ....

_read_blocksetval() is allocating errmsg the first time, two lines later another asprintf() is allocating it the next time.

So I see, that sslh has two problems:

• possibly dangling pointers because of not checking asprintf result
• memory leaks, as not deallocating the asprintf-allocated memory-blobs

I would be glad, if anybody could prove me wrong.

[ftasnetamot]

the good news is: asprintf() is only used in

• log.c
• sslh-conf.c
• echosrv-conf.c

In log.c the implementation is perfect:

    res = asprintf(&name2, "%s[%d]", basename(name1), getpid());
    CHECK_RES_DIE(res, "asprintf");

Once sslh-conf.c is cleaned, the solution can be taken over to echosrv-conf.c, as this seems to be just almost a clone and is only used in testing.

[yrutschle]

Actually, I think it is in fact only important in log.c, which is used during normal run time. The others are only used when reading the configuration file, so:

• failing memory allocation is unlikely
• dangling pointer will result in a crash, but we're terminating anyway and alreay unable to report errors
• memory leak does not matter as we're terminating anyway.

So altogether not a big deal for sslh (besides angering your compiler :) )

That being said, this code is generated by (conf2struct)[https://github.com/yrutschle/conf2struct], which I guess can have use cases where reading a configuration file that fails is not terminal, so it should be handled there. I use asprintf quite a bit over there, so I need to think how to deal with this. I created https://github.com/yrutschle/conf2struct/issues/18 to not forget about this, at least!