reopen_standard_stream contains nonsensical mix of fd-level and FILE-level operations, can corrupt C library state

[zackw]

This part of reopen_standard_stream mixes operations at the fd level with operations at the FILE level in a way that doesn't make any sense to me and, as reported (somewhat incoherently) at https://stackoverflow.com/questions/70879665/ , can wind up corrupting the C library's internal state associated with stdout. Also, several of the error handling paths are incorrect and would cause either crashes or file descriptor leaks.

To the extent I understand what this is trying to do, it looks like a corrected version would be

        if ((fd_copy = dup(fd)) < 0) {
                log_sys_error("dup", name);
                return 0;
        }
        if (!(new_stream = fdopen(fd_copy, mode))) {
                log_sys_error("fdopen", name);
                close(fd_copy);
                return 0;
        }
        flockfile(old_stream);
        fflush(old_stream);
        _check_and_replace_standard_log_streams(old_stream, new_stream);
        *stream = new_stream;
        funlockfile(old_stream);

That is, don't try to close the original FILE object at all, just make a second FILE pointing to a duplicate of the original file descriptor, which the library can use internally as it sees fit.

It's possible that my suggestion doesn't accomplish what this code is supposed to do; if you could explain the actual goal I can maybe give better advice.

[JimmyLin101]

Hello team, Let me explain the steps to reproduce.

The example_cmdlib.c works fine. https://github.com/lvmteam/lvm2/blob/master/doc/example_cmdlib.c
But, stdout will be corrupted when I define a function in example_cmdlib.c like this:

#include "tools/lvm2cmd.h"
#include <stdio.h> 
#include <stdarg.h>

int my_function(const char *formatP, ...)
{
    va_list  ap;
    va_start(ap, formatP);
    vfprintf(stdout, formatP, ap);
    va_end(ap);
    return 0;
}

/* All output gets passed to this function line-by-line */
void test_log_fn(int level, const char *file, int line,
         int dm_errno, const char *format)
{
    /* Extract and process output here rather than printing it */

    if (level != 4)
        return;

    printf("%s\n", format);
    return;
}

int main(int argc, char **argv)
{
    void *handle;
    int r;

    lvm2_log_fn(test_log_fn);

    handle = lvm2_init();

    lvm2_log_level(handle, 1);
    r = lvm2_run(handle, "vgs");

    /* More commands here */

    lvm2_exit(handle);

    return r;
}

The print( ) AIP in test_log_fn doesn't work when I define my_function.

./configure --enable-cmdlib # How I configure LVM

/lib/x86_64-linux-gnu/libc.so.6 # The libc in my running environment

GNU C Library (Ubuntu GLIBC 2.23-0ubuntu11.3) stable release version 2.23

[zkabelac]

The core problem was ensure what kind of buffering glibc will use - and since lvm2 works across numerous version of glibc - various versions seemed to have different problems with our major requirement to use already preallocated buffer (so glibc does not resize/reallocate it's internal stream buffers later while i.e. buffering log output while we do not want any allocation/mmap to happen.

So while the code might look weird we are not aware of any crashes/memory leak with current solution - if there is any - please report as bug with reproducer.

As we no longer support any library usage from lvm (user should stay with call of lvm command - eventually they may try D-Bus - although this API has its limits) - I'm not quite sure what is 2nd. comment about ??