Fixed memory allocation for `struct dirent`

[mager-m]

While porting clixon to the RTOS Blackberry QNX there was memory corruption while reading the yang models from the disk. Debugging led to the function clicon_file_dirent in clixon_file.c in which the struct dirent is copied into an array. According to the UNIX struct dirent documentation:

The name of an array of char of an unspecified size should not be used as an lvalue. Use of:

    sizeof(d_name)

is incorrect; use:

    strlen(d_name)

instead.

I adjusted the memory allocation to take the strlen(dent->d_name) into account.

[s-bauer]

@olofhagsand It was quite a journey, but finally we got clixon running on QNX. Most of this journey evolved around learning the specialties of QNX, digging through documentation and finding a workaround for autotools on Windows. Porting the source code in the end was nothing special and mainly involved:

• Makefile adjustments
• Commenting out privilege drop as setresuid, getresuid and so on are not available on QNX. Not sure if there exist any alternatives. We'll come back to this eventually, but it's not a high priority atm.
• a few import changes

So the baseline is set. From here on the goal is to adapt our implementation of the draft-ietf-tcpm-yang-tcp-01 RFC Draft to work with QNX as this is part of a research project with the University of Applied Sciences in Esslingen. It doesn't end there thought, as this research project is in cooperation with a big Germany corporation which is interested in further evaluating clixon and Netconf/RESTconf/(Coreconf?) and its application scenarios their products.

[olofhagsand]

Commenting out privilege drop as setresuid, getresuid and so on are not available on QNX. Not sure if there exist any alternatives. We'll come back to this eventually, but it's not a high priority atm.

My recommended way is to add a check in autotools so you get HAVE_SETRESUID in the c-code. In that way, if get/setresuid is not available on the platform, drop of privilieges is not supported on those platforms. I can add that in configure.ac right away in master. I have had problems with that elsewhere as well. If there are other modifications (you mention makefile) can you submit those too? The ambition with clixon is to make it as portable to as many platforms as possible. Preferably I would like to include it in the CI, but judging from your comments it may be too windows-based, and in that case it may be too far-fetched. I believe there may also be some licensing/proprietary issues with QNX?

[s-bauer]

That sounds great! It was also our intention to try to merge back as much as possible! From what we've experienced so far it seems that all changes we made can easily be integrated in a generic way. Adding a HAVE_SETRESUID is a good idea. Maybe I can have a shot implementing flag to learn some more about autoconf.

For the CI/CD part: I guess that won't be possible. QNX is not Windows bound and probably works better on Linux (especially because of the configure script) so that's not the issue. However, as you've guessed there will be licensing issues. From what the company told us, the licenses are very expensive and the installation process in a CI/CD environment will probably be too complicated and time consuming as well. I don't think this will matter in the end though, as most of the changes we did are generic and not QNX specific.

[olofhagsand]

OK, I guessed as much regarding the CI.

But just pushing the diffs (if they interoperate with platforms) is fine.

Actually, I pulled this in by mistake and it crashes on eg ubuntu. I dont know the actual reason but it breaks when constructing an array of dirents, eg on the second dirent:

Jun 11 13:30:19: clicon_file_dirent 0 29 280
Jun 11 13:30:19: clicon_file_dirent memcpy(0x591f670 0x5913c80 29
Jun 11 13:30:19: clicon_file_dirent 1 29 280
Jun 11 13:30:19: clicon_file_dirent memcpy(0x591f8c8 0x5913cb8 29
Jun 11 13:30:19: clicon_file_dirent memcpy(0x591f8c8 0x5913cb8 29
==6670== Invalid write of size 8
==6670==    at 0x4C36163: memcpy@GLIBC_2.2.5 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6670==    by 0x4E72B6E: clicon_file_dirent (clixon_file.c:164)
==6670==    by 0x4E92223: yang_spec_load_dir (clixon_yang_parse_lib.c:1612)
==6670==    by 0x109BCA: main (clixon_util_path.c:174)
==6670==  Address 0x591f8c8 is 24 bytes inside an unallocated block of size 4,106,032 in arena "client"
==6670== 

The reason is that there is some misalignment between how the struct size is computed with your code (29) and the original (280). The issue is when creating the second element. maybe it is wrong to create a contiguous array of dirents. I dont see this as being a large memory problem so it may be easier to copy them all over to another array, such as a cvec.

I therefore need to revert that pull to have the CI work, but I will keep the new code in #ifdef:s.

[olofhagsand]

Also the configure change is quite trivial: AC_CHECK_FUNCS(getresuid) which produces a constant in include/clixon_config.h.in / include/clixon_config.h:

/* Define to 1 if you have the `getresuid' function. */
#define HAVE_GETRESUID 1

ANd then use that constant to comment out privileges code. The difficulty is in how to handle the errors that result in if you assume on ahigher layer there is a priviliges drop but the platform cannot do it. I already have prepared a patch.

[s-bauer]

Ah interesting. Actually, how about the clicon_file_dirent function not returning an array of struct dirent but just an array of strings? I've checked all the references and it should be easy to implement!

[olofhagsand]

I reverted it

Author: Olof hagsand <olof@hagsand.se>
Date:   Fri Jun 11 14:05:19 2021 +0200

    revert of https://github.com/clicon/clixon/pull/238

See ifdefs in src/clixon_file.c

[olofhagsand]

yes, but one needs to change the way the calling functions use it:

    char          *dir = "/root/fs";
    struct dirent *dp;
    if ((ndp = clicon_file_dirent(dir, &dp, "(.so)$", S_IFREG)) < 0)
        return -1;
    for (i = 0; i < ndp; i++) 
        do something with dp[i].d_name;
    free(dp);

I see only d_name is used by the callers, so a set of strings would do.

[olofhagsand]

Here is patch for getresuid check

ommit 5ead099d0b92e3631f58a0b0f6ff113e126e1d9d
Author: Olof hagsand <olof@hagsand.se>
Date:   Fri Jun 11 11:02:22 2021 +0200

    * Added autotool check for getresuid (and related functions) necessary for lowering of priviliges for backend and restconf
      * If getresuid is not available, CLICON_RESTCONF_PRIVILEGES must be set to 'none'