fix: panic caused by nil Writer provided to Logger #1228
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
rbtr
reviewed
Feb 8, 2022
log/logger_test.go
Outdated
|
|
||
| // confirm the assumptions of this test before we run it: | ||
| if _, err := os.Stat(fullLogPath); err == nil { | ||
| t.Skipf("The log file at %q exists, so this test cannot sensibly run. Delete it first", fullLogPath) |
Collaborator
There was a problem hiding this comment.
would it be better to use a relative path and always delete it during test setup?
Member
Author
There was a problem hiding this comment.
@rbtr I like the general idea. I did a bit more research and found this SO post. The Go equivalent is os.TempDir which also does the right thing on Linux and Windows, so I'm inclined to use it. Patch forthcoming :)
In the log.(*Logger).Init method, several different strategies are considered based on the configuration provided by the user. In one common case, a rotating log file is opened and the log output is tee-ed into it. If there is any issue with opening that log file, the `log.(*Log).SetOutput` method is never invoked on the underlying logger. Since this standard library logger was created with `nil` passed as the `io.Writer`, this results in a nil pointer deref panic. The error causing the issue is also not surfaced to the user. This initializes the standard library logger with the `io.Discard` writer. This adheres to the original intention here, while also being a safer alternative to nil (effectively writes are to /dev/null). Also, the error is written out to stderr, in the hopes that an operator will be able to diagnose and fix the issue (in the absense of a correct logger configuration). Finally, the logger initialization has been moved to a new function, `log.NewLoggerE` that properly exposes the underlying error. The original function, `NewLogger`, has been modified to use this function instead and has been marked Deprecated.
Collaborator
|
/azp run |
The linter correctly pointed out that the types are redundant here. While ordinarily I tend to leave them if the ellision appears in the middle of the signature, I respect the linter's decision :)
|
Azure Pipelines successfully started running 1 pipeline(s). |
Rather than trying to pull a non-existant path out of thin air and hoping that it doesn't exist, this instead uses os.TempDir to create a guaranteed-empty directory. From within the newly-created tempdir we try to use a non-existent path. This is better both on a single platform, but also has the benefit of working well on Windows too.
timraymond
force-pushed
the
bugfix/log-panic
branch
from
5bbf6ec to
6abe071
Compare
rbtr
requested changes
Feb 8, 2022
Collaborator
rbtr
left a comment
rbtr
left a comment
There was a problem hiding this comment.
deleting the wrong thing in the test, i think
rbtr
requested changes
Feb 8, 2022
Collaborator
rbtr
left a comment
rbtr
left a comment
There was a problem hiding this comment.
forgot to include the comment 🙃
The intention of this test was to create a new temporary directory rather than use the system's temporary directory. This mistake was pointed out by @rbtr in a code review (thanks!).
When an error is present, we want to return both the error and the logger to guarantee that a usable logger is always present. This returns the logger in all circumstances and documents that fact in the godoc for this function.
Member
Author
|
/azp run |
|
Commenter does not have sufficient privileges for PR 1228 in repo Azure/azure-container-networking |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ramiro-gamarra
approved these changes
Feb 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reason for Change:
In the log.(*Logger).Init method, several different strategies are
considered based on the configuration provided by the user. In one
common case, a rotating log file is opened and the log output is tee-ed
into it. If there is any issue with opening that log file, the
log.(*Log).SetOutputmethod is never invoked on the underlying logger.Since this standard library logger was created with
nilpassed as theio.Writer, this results in a nil pointer deref panic. The errorcausing the issue is also not surfaced to the user.
This initializes the standard library logger with the
io.Discardwriter. This adheres to the original intention here, while also being a
safer alternative to nil (effectively writes are to /dev/null). Also,
the error is written out to stderr, in the hopes that an operator will
be able to diagnose and fix the issue (in the absense of a correct
logger configuration).
Finally, the logger initialization has been moved to a new
function,
log.NewLoggerEthat properly exposes the underlying error.The original function,
NewLogger, has been modified to use thisfunction instead and has been marked Deprecated.
Issue Fixed:
Fixes #1227
Requirements: