8 ms·
Though of course "debug.log(stuffIGotFromPeer)" is also very common (and as you point out should always be avoided).
by janekm 5y ago
Though of course "debug.log(stuffIGotFromPeer)" is also very common (and as you point out should always be avoided).
- lvh 5y agoFortunately (and for reasons I can't begin to understand), debug and info logging levels seem to be safe, but error is not. Technically better, but somehow... morally worse?
- Pxtl 5y agoIf "debug.log(stuffIGotFromPeer)" is dangerous, then the problem is with debug.log and not my code. Safe logging is not too high of a bar.
- jhugo 5y agoThe point is that the first argument is the format string. debug.log("{}", stuffIGotFromPeer) should generally be safe (this bug is an example of when it isn't, though)
- jameshart 5y agoThe fact that the first argument is interpreted as a format string even when you aren't supplying format arguments is a violation of 'principle of least surprise'. The availability of format parameters that can access environment data - let alone remotely loaded code - is a feature most people won't discover unless they go looking for it.
- rixed 5y agoHow are varargs implemented in the JVM ABI? Is the called function even aware of how many parameters were actually passed, or does it have to rely on the format string for that?
- jameshart 5y agoTurned into an object[] array. The log4j API implements a few overloads of each log method to help avoid the implicit array allocation happening in common logging cases (no args, one arg, etc). The call site can tell if args were provided.
- watwut 5y agoThe format string should not allow remote execution unless explicitly tuned on either. Logging should not be the thing you have to sanitize.
- smsm42 5y agoTBH, this is a huge antipattern. You have to put some context in your logs, so it must be something like "debug.log("Got from peer: {}", stuffIGotFromPeer) or something like that. You can't complain printf(stuffIGotFromPeer) is dangerous, so here is the same. But in log4j, debug.log("Got from peer: {}", stuffIGotFromPeer) is as unsafe as the other one, too! There's no way to make it safe, as I understand.
- spion 5y agoDon't see why not. Evaluate the `${}` stuff first, then do the `{}` substitutions.