Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leftover logfile is close()d … unpredictably? #39

Closed
FGasper opened this issue Jun 9, 2019 · 5 comments

Comments

@FGasper
Copy link

commented Jun 9, 2019

#include <stdio.h>
#include <unbound.h>

int main(void)
{
    FILE *tfh = tmpfile();
    setvbuf( tfh, NULL, _IONBF, 0 );

    struct ub_ctx* ctx;

    for (int i=0; i<2; i++) {
        ctx = ub_ctx_create();

        ub_ctx_debuglevel(ctx, 5);
        ub_ctx_debugout( ctx, tfh );

        ub_ctx_delete(ctx);

        if (-1 == fseek(tfh, 0, 0)) {
            perror("fseek()");
        }
    }
}

The above code illustrates a problem where a 2nd context creation will clobber a leftover logfile from a former context.

It seems like the ideal fix would be to put the log state variables into the context struct rather than having them be globals?

(I haven’t tested it, but it looks like, given the globals, it would be problematic, for example, to have two concurrent Unbound objects in the same process with different log outputs?)

Failing that change … is there any way to fix this, short of removing that fclose() in log.c (and potentially breaking behavior that implementations may have been expecting now for over 10 years)?

Regardless, it seems like this issue should at least be documented? The existing libunbound(3) documentation for ub_ctx_debugout() gives the impression that log state is contained entirely within the log object, which of course isn’t the case.

The workaround seems to be to set the log handle to stderr before deleting the context … does that seem reasonable?

Thank you!

@wcawijngaards

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Have you tried _IOLBF ? The line buffer should correspond nicely to the line-at-a-time log entries, when you use two contexts at the same time.
This is a fix to switch to stderr when there is an override, in ctx_delete. It works, and if I add fclose(tfh) to the end of the example to free that, it is error free.

diff --git a/libunbound/libunbound.c b/libunbound/libunbound.c
index 5f5a8a10..9dd7d6db 100644
--- a/libunbound/libunbound.c
+++ b/libunbound/libunbound.c
@@ -328,6 +328,8 @@ ub_ctx_delete(struct ub_ctx* ctx)
        ub_randfree(ctx->seed_rnd);
        alloc_clear(&ctx->superalloc);
        traverse_postorder(&ctx->queries, delq, NULL);
+       if(ctx->logfile_override)
+               log_file(NULL);
        free(ctx);
 #ifdef USE_WINSOCK
        WSACleanup();
@FGasper

This comment has been minimized.

Copy link
Author

commented Jun 13, 2019

This is the workaround that I have in place in my calling code. It seems to fix this particular issue, yes. :)

There seems to be a related problem, where anything that depends on the global logfile will break if there are concurrent contexts:

struct ub_ctx* ctx1 = ub_ctx_create();
struct ub_ctx* ctx2 = ub_ctx_create();

ub_ctx_debugout( ctx1, fh1 );    // sets global `logfile` variable in log.c

ub_ctx_debugout( ctx2, fh2 );    // sets global `logfile` … clobbers previous setting?

I’ve not analyzed the code enough to determine if this causes “real” breakage.

@wcawijngaards

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Not really "real" breakage, apart from a global variable. At least I think that is what happens.

@wcawijngaards

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Thanks for the report, the fix is for the unpredictable close, and does not address the global variable use. That is for debug.

@FGasper

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

Thank you!

jedisct1 added a commit to jedisct1/unbound that referenced this issue Jun 29, 2019
Merge remote-tracking branch 'nlnet/master'
* nlnet/master: (22 commits)
  Nicer spelling and layout.
  - For NLnetLabs#45, check that 127.0.0.1 and ::1 are not used in unbound.conf   when do-not-query-localhost is turned on, or at default on,   unbound-checkconf prints a warning if it is found in forward-addr or   stub-addr statements.
  - Fix memleak in unit test, reported from the clang 8.0 static analyzer.
  - Fix python dict reference and double free in config.
  - Merge PR NLnetLabs#6: Python module: support multiple instances - Merge PR NLnetLabs#5: Python module: define constant MODULE_RESTART_NEXT - Merge PR NLnetLabs#4: Python module: assign something useful to the   per-query data store 'qdata' Noted in Changelog.
  - Added documentation to the ipset files (for doxygen output).
  - make depend
  - Fix to make unbound-control with ipset, remove unused variable,   use unsigned type because of comparison, and assign null instead   of compare with it.  Remade lex and yacc output.
  - PR NLnetLabs#28: IPSet module, by Kevin Chou.  Created a module to support   the ipset that could add the domain's ip to a list easily.   Needs libmnl, and --enable-ipset and config it, doc/README.ipset.md. - Fix to omit RRSIGs from addition to the ipset.
  - Fix for NLnetLabs#24: Fix abort due to scan of auth zone masters using old   address from previous scan.
  - Fix NLnetLabs#39: In libunbound, leftover logfile is close()d unpredictably.
  - Master contains version 1.9.3 in development.
  fix segmentation fault
  rollback the code
  bugfix
  performance improvement
  edit config parser to support ipset
  Add support for ipset
  Document how to configure multiple python modules
  Support multiple python module instances
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.