Be able to turn logging on/off through environment about pwntools HOT 10 CLOSED

Upvote

from pwntools.

While I agree that this is an issue, I really think that this is just a part of a more fundamental issue with logging in pwntools.

@kokjo have been arguing for a while we should use built-in logging module from python, and I mostly agree with him. While I know almost nothing about the built-in module, we should not reinvent the wheel, unless our wheel is significantly better. As far as I know, doing this would at least solve the problem Robert mentioned.

In my opinion, logging in pwntools consists of three parts (though they are somewhat muddled together):

• The API for doing logging. E.g. if I want to log some non-critical information, I call pwnlib.log.info with arguments so and so. (Henceforth called the "output API")
• The API for controlling what pieces of logging goes where. Currently this mostly consists of pwnlib.context.log_level, but also consists of some of the parsing of sys.argv in pwn/init.py. (Henceforth called the "control API")
• The way things are drawn. E.g. an info-message is prepended by a blue *. (Henceforth called the backend)

In @kokjo's suggestion, the first two either go away completely or become thin wrappers around the built-in logging module and the last one becomes a backend-module for the built-in logging module.

Before deciding if we should do this, I think we should for each of those points look at:

• Is the built-in module as feature-complete as we are. (Or can we live with the loss of features?)
• Is the built-in module as easy or nearly as easy to use as our current code. (Or can it be fixed by creating thin wrappers?)

The output API

There are two obvious features, which will in all likelihood not be in the built-in module.

First of all, there are some functions, which will probably not be in the built-in module. These are at least: indented, error, bug, fatal, stub, waitfor and status. I find it acceptable to keep these. All except the last two can be implemented as very thin wrappers around the built-in module and the last two does not need to use the built-in module at all.

Second of all, it will not have the arguments frozen, float, priority or indent, and it will not return a handle. With the exception of indent, I think this is completely acceptable. If you want to use these features, just use pwnlib.term.output instead. I can also accept the loss of indent, especially if we keep indented-function.

I would like to know if there is anything else that the built-in module is missing feature- or usability-wise, but I do not know it well enough or have the time to check it out right now.

The control API

I expect the built-in module to be much stronger in this regard. I mostly have one issue: Would it be possible to implemented a per-thread function similar to:

with context.local(log_level = 'silent'):
    do_stuff()

Again I would like to know, if we are missing other stuff in terms of features or usability, but do not have the time right now.

The backend

Well, I cannot see any reason why the backend part should not work just as well as it does currently. It would get a message, format it and output using either term.output or sys.stderr.write depending on whether we are in term_mode or not.

Can anybody see any potential issues in this part?

Conclusion

I am very hesitant to simply "fix" the current problem without thinking. Before doing anything, I would really like the have the open questions in this post answered, along with more overall question: Do we want to change the way we do things?

If somebody were to answer all my questions with facts, then I would not mind implementing the changes required, whether it be using the built-in module or doing a hack with log files.

from pwntools.

On the with statement, it's pretty straightforward.

from logging import *

# Assume we have a global 'log' object as we
# do now (though currently it's a module.)
basicConfig(level=INFO)
log = getLogger(__name__)


class LogContext(object):
    def __init__(self, level):
        self.level = level
    def __enter__(self):
        global log
        self.old_level = log.getEffectiveLevel()
        log.setLevel(self.level)
    def __exit__(self, *args):
        global log
        log.setLevel(self.old_level)


log.info("INFO outside 'with'")
log.debug("DEBUG outside 'with'")
with LogContext(DEBUG):
    log.debug("DEBUG inside 'with'")

from pwntools.

I'm in favor of a rewrite of the current log module to use the real logging mechanism.

from pwntools.

In case it was not clear: I am mostly in favor too, but not before we figure out how to solve at least the few kinks I mentioned.

@zachriggle: Would that method be thread-safe? E.g. can I do that in one thread, while another keeps printing info messages?

from pwntools.

I don't know the built in logging either, but there is output in pwntools that I would not call logging but rather status indication. The animations for example. Some might call them bloat and unnecessary, but I really like them, especially when the framework is doing something under the hood that takes a long time, such as most of what DynELF does. It makes me know, that something is running (or not!).
Colors are also very useful.

However, when on sysadmin duty during a CTF, none of the output is really useful to me, thou it may be to the guy who actually wrote the exploit in case something goes wrong, so being able to turn off animations and write the rest to a file, or more likely one file per run, would be useful. The attack framework should do its own logging that will likely be more interesting to admins.

from pwntools.

@RobertLarsen I agree -- those are specifically the waitfor and status functions that I mentioned above.

from pwntools.

Yep, all agreed. Re: Thread-safety for log levels, it'd need to be hacked-in as things are in the current manner. log would need to resolve to a property on a types.ModuleType instance, very similar to how context is done.

The biggest issue, I think, will be reconciling the waitfor behavior and use of the logging mechanism.

from pwntools.

So I've now merged in my enhancements to the logging framework. The logging all goes through the standard Python logging interface. It can be hooked, modified, or rerouted at will.

Is the goal of this issue to have greater control over the logging, or just to actually be able to shut it up with an environment variable?

from pwntools.

Looks like @MortenBP added environment variables in aab7122

from pwntools.