Simplest code triggers assertion failure about ddcutil HOT 9 CLOSED

If I use DDCA_INIT_OPTIONS_DISABLE_CONFIG_FILE, the error is gone. However the console prints unnecessary messages:

libddcutil: Options passed from client: --disable-watch-displays
libddcutil: Applying combined options: --disable-watch-displays --disable-watch-displays

which ruined my program

And the ASAN complains

=================================================================
==7903==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 101 byte(s) in 1 object(s) allocated from:
    #0 0x7f4a81ae007a in __interceptor_realloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
    #1 0x7f4a8181c3ad  (/usr/lib/libc.so.6+0xfa3ad) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f4a81ae1359 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f4a810e6712 in g_malloc (/usr/lib/libglib-2.0.so.0+0x62712) (BuildId: 1916d89bc0f8f0932e584f87427c2fedfc8a293b)

SUMMARY: AddressSanitizer: 133 byte(s) leaked in 2 allocation(s).

Full code

// gcc test.c -o test -lddcutil -g -fsanitize=address
#include <ddcutil_c_api.h>

int main()
{
  ddca_init("--disable-watch-displays", DDCA_SYSLOG_NOT_SET, DDCA_INIT_OPTIONS_DISABLE_CONFIG_FILE);
}

from ddcutil.

Ref: #330 (comment)

from ddcutil.

I have pushed a fix for the assert() failure to branch 2.0.2-dev.

Running your sample program under valgrind shows some leaks. I will investigate.

Re the options messages, I see your problem. The messages exist in order to avoid confusion about unexpected settings, but they should be optional in some way. In retrospect ddca_init() should have had an additional argument at which a null-terminated list of benign initialization messages are returned. I will have to think through how to control those messages without a SONAME bump. In the meantime, wrapping ddca_init() in ddca_start_capture() and ddca_end_capture() should address the problem, though it is kludgy.

Re the reference to issue #330, are you saying the dlopen() segfault problem still exists? Does it require that driver i2c-dev not be loaded?

Finally, I see that you're running your program in a VM using the VMWare virtualized video driver. Last I looked at the question some time ago, virtualized video drivers do not support I2C communication. The only way to access the /dev/i2c devices from a VM is to use a non-virtual video driver using passthru to an actual physical video card.

from ddcutil.

Thanks for the fix.

I will have to think through how to control those messages without a SONAME bump. In the meantime, wrapping ddca_init() in ddca_start_capture() and ddca_end_capture() should address the problem, though it is kludgy.

What about a new flag --no-verbose?

Re the reference to issue #330, are you saying the dlopen() segfault problem still exists? Does it require that driver i2c-dev not be loaded?

I metioned the issue because it was the source that --disable-watch-displays came from. I don't know whether the assertion failure was caused by the flag or not, because I can't find the flag in the doc.

Finally, I see that you're running your program in a VM using the VMWare virtualized video driver. Last I looked at the question some time ago, virtualized video drivers do not support I2C communication. The only way to access the /dev/i2c devices from a VM is to use a non-virtual video driver using passthru to an actual physical video card.

I knew it. The Linux distro I use (Fedora) haven't upgrade ddcutil to 2.0 yet, so I tested it in a VM with a different distro.

AFAIK most distros still stuck with ddcutil 1.4, so my program has to support both 1.4 and 2.0. I have to say that it was not a good experience. Because of the API imcompatibility, my code ends up with

#if DDCUTIL_VMAJOR >= 2
double ddca_set_default_sleep_multiplier(double multiplier);
#else
void ddca_init(const char* libopts, int syslog_level_arg, int opts);
#endif

void pseudoCode()
{
    void* libddcutil = dlopen("libddcutil.so");
    __typeof__(&ddca_init) ffddca_init = dlsym(libddcutil, "ddca_init");
    if (ffddca_init)
    {
        // use ffddca_init
    }
    else
    {
        __typeof__(&ddca_set_default_sleep_multiplier) ffddca_set_default_sleep_multiplier = dlsym(libddcutil, "ddca_set_default_sleep_multiplier");
        if (ffddca_set_default_sleep_multiplier)
        {
            // use ffddca_set_default_sleep_multiplier
        }
    }
    dlclose(libddcutil);
}

In addition, ddca_init is not strongly typed and the supported flags are hard to find. Passing dynamic values requires snprintf, which is ugly too.

from ddcutil.

I have added option flag DDCA_INIT_OPTIONS_ENABLE_INIT_MSGS to DDCA_Init_Options in branch 2.0.2-dev. The informational messages regarding how the option string is assembled will only be output if this flag is set. (These messages are still written to the system log, if syslog is enabled.) As I previously noted, a better solution would be to add a msg collector argument to ddca_init() but that would have required a SONAME bump, and adding a new ddca_init2() function with that argument would I think be needlessly confusing.

Why is ddca_init() defined the way it is? The number of ddcutil options is vast. Most are useful only for various special cases. Having rarely used API calls for each option bloated the API, and the problem was only going to get worse. Plus the options passed from the client program must be merged in some way from those obtained from the configuration file, and some settings are applicable only before initialization has occurred. So the general solution is to assemble an option string from the configuration file and the libopts string argument to ddca_init() and pass that assembled string to the parser.

However, some things must be known before the parser is called. which is why ddca_init() has the syslog_level and opts arguments. In particular, that is why the informational messages about how the options string has been assembled are controlled by a flag in opts instead of the ultimate option string passed to the parser.

I have added documentation about option --disable-watch-displays to page Shared Library Options. The default may change to --enable-watch-displays in a future release, so for safety I'd suggest just continuing to pass this option as dynamic display detection is irrelevant for your application.

I apologize for the complexity release 2.0 adds to your application. Given the need for a SONAME bump, this was the time to bite the bullet on a large number of needed changes. I am aware that it will be some time before ddcutil 2.0 will make it into most distributions. I maintain ddcutil in the official Debian repositories, and my plan is to continue to distribute libddcuti.so.4 as well as the new libddcutil.so.5. I would hope that other binary based distributions do the same. This should enable existing applications that do not use dynamic linking to work unchanged. (As the process for acceptance of packages into Debian is complex, I do not plan to submit ddcutil 2.0 until at least the first patch release. It should then become part of Ubuntu long term support release 24.04.)

Regarding memory leaks, the ASAN output gives no hint as to the code that is causing the leak. More useful would be to build libddcutil with CFLAGS=-g and run your program under valgrind with option --leak-check=full. If you'd prefer, direct me to your source code I will do this myself.

Finally, thank you for your precise input. You're using libddcutil in a way I hadn't anticipated, so it's very helpful and I want to be responsive to the problems you encounter.

from ddcutil.

If you'd prefer, direct me to your source code I will do this myself.

You don't need my code to reproduce the leaks. Just call ddca_init(whatever) and ASAN will complain.

I would hope that other binary based distributions do the same. This should enable existing applications that do not use dynamic linking to work unchanged.

Some distro / package managers have adopted ddcutil 2.0. If your program has incompatibility issues against their existing packages, it's your fault and it's your duty to resolve the issues, despite you did nothing wrong.

Anyway, My code is here if you interest: https://github.com/fastfetch-cli/fastfetch/blob/dev/src/detection/brightness/brightness_linux.c#L93-L148 (Usage of ddca_init was removed because of the unexpected messages)

My requirement is simple. It fetches brightness values of all external displays as fast as possible. And if the user doesn't have libddcutil installed, the function will be ignored.

from ddcutil.

carter@carter-vmwarevirtualplatform ~/test> cat test.c
// gcc test.c -o test -lddcutil -g -fsanitize=address
#include <ddcutil_c_api.h>

int main()
{
  ddca_init("--disable-watch-displays", DDCA_SYSLOG_NOT_SET, DDCA_INIT_OPTIONS_DISABLE_CONFIG_FILE);
}
carter@carter-vmwarevirtualplatform ~/test> make
gcc test.c -o test -lddcutil -g -fsanitize=address -fno-omit-frame-pointer
carter@carter-vmwarevirtualplatform ~/test> ASAN_OPTIONS=fast_unwind_on_malloc=0 ./test
libddcutil: Options passed from client: --disable-watch-displays
libddcutil: Applying combined options: --disable-watch-displays --disable-watch-displays

=================================================================
==10391==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 101 byte(s) in 1 object(s) allocated from:
    #0 0x7f7eb52e007a in __interceptor_realloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
    #1 0x7f7eb51183ad  (/usr/lib/libc.so.6+0xfa3ad) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #2 0x7f7eb511c36d in wordexp (/usr/lib/libc.so.6+0xfe36d) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #3 0x7f7eb52ae141 in __interceptor_wordexp /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:4237
    #4 0x7f7eb591a6b9  (/usr/lib/libddcutil.so.5+0x6e6b9) (BuildId: bda49262b293013d5bbb46da7dde75200f49267c)
    #5 0x7f7eb593748e in ddca_init (/usr/lib/libddcutil.so.5+0x8b48e) (BuildId: bda49262b293013d5bbb46da7dde75200f49267c)
    #6 0x5599feb21195 in main /home/carter/test/test.c:6
    #7 0x7f7eb5045ccf  (/usr/lib/libc.so.6+0x27ccf) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #8 0x7f7eb5045d89 in __libc_start_main (/usr/lib/libc.so.6+0x27d89) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #9 0x5599feb210a4 in _start (/home/carter/test/test+0x10a4) (BuildId: ff0ee81df3e40c717a3ab619e6083bd1f05b2d1b)

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f7eb52e1359 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f7eb49d3712 in g_malloc (/usr/lib/libglib-2.0.so.0+0x62712) (BuildId: 1916d89bc0f8f0932e584f87427c2fedfc8a293b)
    #2 0x7f7eb4994137 in g_ptr_array_new_with_free_func (/usr/lib/libglib-2.0.so.0+0x23137) (BuildId: 1916d89bc0f8f0932e584f87427c2fedfc8a293b)
    #3 0x7f7eb5937519 in ddca_init (/usr/lib/libddcutil.so.5+0x8b519) (BuildId: bda49262b293013d5bbb46da7dde75200f49267c)
    #4 0x5599feb21195 in main /home/carter/test/test.c:6
    #5 0x7f7eb5045ccf  (/usr/lib/libc.so.6+0x27ccf) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #6 0x7f7eb5045d89 in __libc_start_main (/usr/lib/libc.so.6+0x27d89) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)
    #7 0x5599feb210a4 in _start (/home/carter/test/test+0x10a4) (BuildId: ff0ee81df3e40c717a3ab619e6083bd1f05b2d1b)

SUMMARY: AddressSanitizer: 133 byte(s) leaked in 2 allocation(s).
carter@carter-vmwarevirtualplatform ~/test [1]> 

from ddcutil.

The most recent memory leaks you have reported should now be addressed in branch 2.0.2-dev.

Re:

Some distro / package managers have adopted ddcutil 2.0. If your program has incompatibility issues against their existing packages, it's your fault and your duty to resolve the issues.

No. The SONAME change from libddcutil.4 to libddcutil.5 declares that the new library is not backwards compatible with the old. Applications written to the earlier library should require that library. I will try to make it clearer to distributions that both libraries need to be installed if they have applications built to the older library, and simpler to do so, but that is their responsibility.

from ddcutil.

Hopefully a new release soon

from ddcutil.