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

Race Condition with SetReportCaller() #954

Closed
jeremyje opened this issue Apr 17, 2019 · 4 comments
Closed

Race Condition with SetReportCaller() #954

jeremyje opened this issue Apr 17, 2019 · 4 comments

Comments

@jeremyje
Copy link

It looks like there's a data race condition between SetReportCaller and internal reads from entry.go.

https://github.com/sirupsen/logrus/blob/master/entry.go#L220 and https://github.com/sirupsen/logrus/blob/master/entry.go#L200 are 2 probable culprits here.

Step #8 - "Build: Test": ==================
Step #8 - "Build: Test": WARNING: DATA RACE
Step #8 - "Build: Test": Write at 0x00c00003c208 by goroutine 67:
Step #8 - "Build: Test":   github.com/sirupsen/logrus.(*Logger).SetReportCaller()
Step #8 - "Build: Test":       /go/pkg/mod/github.com/sirupsen/logrus@v1.4.1/logger.go:341 +0x85
Step #8 - "Build: Test":   github.com/GoogleCloudPlatform/open-match/internal/serving/testing.createOpenMatchServer()
Step #8 - "Build: Test":       /go/pkg/mod/github.com/sirupsen/logrus@v1.4.1/exported.go:31 +0xf6
Step #8 - "Build: Test":   github.com/GoogleCloudPlatform/open-match/internal/serving/testing.NewMiniMatch()
Step #8 - "Build: Test":       /workspace/internal/serving/testing/minimatch.go:65 +0x6d
Step #8 - "Build: Test":   github.com/GoogleCloudPlatform/open-match/test/e2e/frontend.TestFrontendStartup()
Step #8 - "Build: Test":       /workspace/test/e2e/frontend/frontendapi_test.go:13 +0x76
Step #8 - "Build: Test":   testing.tRunner()
Step #8 - "Build: Test":       /usr/local/go/src/testing/testing.go:865 +0x163
Step #8 - "Build: Test": 
Step #8 - "Build: Test": Previous read at 0x00c00003c208 by goroutine 62:
Step #8 - "Build: Test":   github.com/sirupsen/logrus.Entry.log()
Step #8 - "Build: Test":       /go/pkg/mod/github.com/sirupsen/logrus@v1.4.1/entry.go:220 +0x1c2
Step #8 - "Build: Test":   github.com/sirupsen/logrus.(*Entry).Log()
Step #8 - "Build: Test":       /go/pkg/mod/github.com/sirupsen/logrus@v1.4.1/entry.go:268 +0x160
Step #8 - "Build: Test":   github.com/GoogleCloudPlatform/open-match/internal/serving.(*GrpcWrapper).Start.func1()
Step #8 - "Build: Test":       /go/pkg/mod/github.com/sirupsen/logrus@v1.4.1/entry.go:297 +0x359
Step #8 - "Build: Test": 
Step #8 - "Build: Test": Goroutine 67 (running) created at:
Step #8 - "Build: Test":   testing.(*T).Run()
Step #8 - "Build: Test":       /usr/local/go/src/testing/testing.go:916 +0x65a
Step #8 - "Build: Test":   testing.runTests.func1()
Step #8 - "Build: Test":       /usr/local/go/src/testing/testing.go:1157 +0xa8
Step #8 - "Build: Test":   testing.tRunner()
Step #8 - "Build: Test":       /usr/local/go/src/testing/testing.go:865 +0x163
Step #8 - "Build: Test":   testing.runTests()
Step #8 - "Build: Test":       /usr/local/go/src/testing/testing.go:1155 +0x523
Step #8 - "Build: Test":   testing.(*M).Run()
Step #8 - "Build: Test":       /usr/local/go/src/testing/testing.go:1072 +0x2eb
Step #8 - "Build: Test":   main.main()
Step #8 - "Build: Test":       _testmain.go:84 +0x334
Step #8 - "Build: Test": 
Step #8 - "Build: Test": Goroutine 62 (running) created at:
Step #8 - "Build: Test":   github.com/GoogleCloudPlatform/open-match/internal/serving.(*GrpcWrapper).Start()
Step #8 - "Build: Test":       /workspace/internal/serving/grpcserver.go:61 +0x581
Step #8 - "Build: Test":   github.com/GoogleCloudPlatform/open-match/internal/serving/testing.NewMiniMatch()
Step #8 - "Build: Test":       /workspace/internal/serving/omserver.go:28 +0xfd
Step #8 - "Build: Test":   github.com/GoogleCloudPlatform/open-match/test/e2e/frontend.TestFrontendStartup()
Step #8 - "Build: Test":       /workspace/test/e2e/frontend/frontendapi_test.go:13 +0x76
Step #8 - "Build: Test":   testing.tRunner()
Step #8 - "Build: Test":       /usr/local/go/src/testing/testing.go:865 +0x163
@jeremyje
Copy link
Author

The fix has a risk to be breaking because ReportCaller bool is a public field on the Logger structs. This field being set is protected by a mutex but the reads are not.

@stale
Copy link

stale bot commented Feb 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 26, 2020
@AlekSi
Copy link

AlekSi commented Feb 26, 2020

It is a real unfixed problem.

@markphelps
Copy link
Collaborator

Fixed by #1116

cgxxv pushed a commit to cgxxv/logrus that referenced this issue Mar 25, 2022
kevinliu0430 added a commit to 17media/logrus that referenced this issue Mar 17, 2025

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
* Fix typo in docs for New()

* Fix error formatting based on best practices from Code Review Comments

Signed-off-by: CodeLingo Bot <bot@codelingo.io>

* Add a CallerPrettyfier callback to the json formatter

* Add a CallerPrettyfier callback to the text formatter

* Remove debug trace

* Add and example for CallerPrettyfier

* fix ReportCaller race condition

* fix sync.Once usage instead of adding a mutex lock

* Add WithContext

* Add CHANGELOG for v1.4.0

* Rewrite if-else-if-else to switch statements

* Add hook to send logs to custom writer sirupsen#678

* Fix some test conditions

* Add Bytes() method to Entry, and use it to avoid double type cast

* Add Go 1.12 to Travis CI build matrix

* Got rid of IsTerminal call to reduce external dependencies

* Removed golang.org/x/crypto refs

* Moved moved unix-related parts into terminal

* Updated travis.yml

* Move terminal package

fixes issue where terminal_check_notappengine.go can't access terminal
package since terminal package is in an internal package

* Test more platforms

It would be 5 * 3 = 15 runs

* return new entry for Entry.WithContext

* Move files to main directory

* Make isTerminal un-exported

* remove field if val is empty string for func and file field in text formatter

* Release 1.4.1

* Fix solaris build

* Update x/sys/unix to fix AIX support

* remove go 1.10 from ci build matrix

* Update README.md

* Add a checkTerminal for nacl to support running on play.golang.org

* add full cross compilation in travis (sirupsen#963)

* add full cross compilation in travis

* reduce the travis build matrix

* disable cross build for plan9 and nacl

* fix build break for plan9

* Release 1.4.2

* tracking commit

* tracking commit

* add implementation and tests

* update comments

* update the readme

* add a space back

* wording shift

* bump ci

* dynamically space the level text

* init the loggers in tests

* avoid escapes! h/t @therealplato

* len => RuneCount

note: this is not currently easily testable without a larger diff that refactors the levels

* its => it's

* Fixed ineffectual assignment in test

Don't assign l if it's not being checked/used afterwards.

* Avoid unnecessary conversion

No need to convert here.

* readme: we have great maintainers now

* return early

This makes it easier to read / understand and is more idiomatic.

* some minimal documentation for Logger.Writer{,Level}

This also includes two examples extracted from the readme.

* Add terminal_check_js.go file, for compatibility with GopherJS

* ReadMe.md file adds The Third Formatter link

* Fixed some typos in README.md

Fixed a few typos and grammatical issues in the README.md. I hope this is helpful just to make a small improvement.

* add Go 1.13 in travis

* Disable modules, run on osx

* go mod verify; go mod tidy

* Enable all of these to see what fails

* get some other deps

* pull all the install into a single location

* This should make gox a little nicer

* Exclude go1.13.x from modules off, only build all on go1.13 modules on

* Associate this example with what it's an example for

* test the error to ensure there isn't an unexpected error in the test

* deadcode

* fix broken test

* Force Quote

Closed sirupsen#1005

* fix race conditions on entry

closes sirupsen#1046

* run golangci-lint on travis

* remove go1.11.x from travis build matrix

* add x rights on travis/lint.sh

* remove obsolete documentation

* improve Logger.WithField documentation

* Fix entity data bleed when using WithContext and WithTime

Creates a copy of the data map when using WithContext to create a
child entity.  Without this, the data map of the parent entitiy,
which is exposed in the entity struct, is shared between a parent
and all children instances.

This can create bugs of shared or overwritten data when a parent
entity is used to make children in differing contexts, and behaves
differently than `WithField` and its diritivites which does make
a copy of the data.

Additionally implements the same logic for WithTime, for API
consistency in behavior.

* Make Entry WithContext and WithTime copy tests more clear

Clarifies the data used in the EntryWithContextCopiesData test and
adds an equivalent test to verify the behavior of WithTime.

* Add support for freebsd/arm64

* Fix typo

* add caption-json-formatter

* Remove annoying punctuation in Readme for better screen reader accessibility.

One entry in the Logrus formatters list in the readme contained
a lot of extraneous punctuation.

When read with a screen reader, nothing but a bunch of question marks and weird symbol names could be heard,
making the line impossible to understand.

* readme: maintenance-mode

* Create stale.yml

* Update stale.yml

* Only mark issues as stale for now until we go through backlog of PRs

Only mark issues as stale for now until we go through backlog of PRs

* Get right logrus package name

* run CI for go 1.13 and 1.14

* Fix wrong caller

* Removed useless files

* Title updates

Removed the non-breaking spaces in the ReadMe

* resolved conflicts

* create test to prove issue sirupsen#954

* fix race condition in entry

* fix deadlock in previous entry race condition fix

* remove errant whitespace

* Add loggers that take functions as input

* Revert sirupsen#1047

* Adds flag to disable quotes in TextFormatter

* Adds additional test cases for DisableQuote

* Change CRLF line endings to LF

Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>

* update github.com/konsorten/go-windows-terminal-sequences dependency to v1.0.3

* complete documetation on TextFormatter.DisableQuote

* update CHANGELOG.md with 1.5.0 and 1.6.0 version contents

* Simplify checkIfTerminal for Windows

Instead of relying on EnableVirtualTerminalProcessing from
github.com/konsorten/go-windows-terminal-sequences which just calls
GetConsoleMode, sets ENABLE_VIRTUAL_TERMINAL_PROCESSING and calls
SetConsoleMode with the new modified mode, implement it directly inside
checkIfTerminal. This also avoids the duplicate call to GetConsoleMode.

* Improve tests for logger.*Fn functions

* Update doc for new logger

Update default formatter for new logger from JsonFormatter to TextFormatter

* Add an API to plug a custom buffer free item mangement system

* update changelog with v1.7.0

* Replace %v with %w for error

https://golang.org/pkg/fmt/#Errorf

* bump golang versions in travis ci

* bump golangci-lint version

* fix linter errors

* one more linter error fixed

* Add build tag to enable a successful build for zos

* desactivate stale bot

* migrate cross build target from bash script to mage

* Remove dead panic in Entry.Panic

[Entry.log itself panics][0] when the log level is set to PanicLevel, (and
PanicLevel is always eneabled) so this second panic will never be reached.

[0]: https://github.com/sirupsen/logrus/blob/8ae478eb8a850a54ea4915a2b572f377de1f9c7e/entry.go#L253

* migrate lint script to a mage target

* add a test target in the magefile

* update travis scripts

* bump to go 1.16
* remove unneeded part in travis/install.sh

* undo golang version bump

* fix for entry data field race condition

* update changelog

* code and comments clean up

* update changelog

* fix race condition AddHook and traces

* travis: run mage with -v to not discard output

Before this:

    $ go run mage.go lint
    Error: running "/Users/sebastiaan/go/bin/golangci-lint run ./..." failed with exit code 1
    exit status 1

    $ go run mage.go test
    Error: running "go test -race -v ./..." failed with exit code 1
    exit status 1

After this:

    $ go run mage.go -v lint
    Running target: Lint
    exec: /Users/sebastiaan/go/bin/golangci-lint run ./...
    entry.go:89:6: `iamNotUsed` is unused (deadcode)
    func iamNotUsed() {
         ^
    Error: running "/Users/sebastiaan/go/bin/golangci-lint run ./..." failed with exit code 1
    exit status 1

    $ go run mage.go -v test
    Running target: Test
    exec: go test -race -v ./...
    === RUN   TestRegister
    ...
    ?   	github.com/sirupsen/logrus/internal/testutils	[no test files]
    FAIL
    Error: running "go test -race -v ./..." failed with exit code 1
    exit status 1

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

* move "mage" to a separate module

Move the magefile related files to a submodule, so that it
does not become a dependency for logrus itself.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

* go.mod: update golang.org/x/sys to fix openbsd/mips64 on Go 1.16

This should hopefully fix cross-compile on openbsd/mips64 on Go 1.16

    Building for GOOS=openbsd GOARCH=mips64
    # golang.org/x/sys/unix
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/fcntl.go:26:42: undefined: Flock_t
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/ioctl.go:26:47: undefined: Winsize
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/ioctl.go:37:47: undefined: Termios
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/ioctl.go:55:42: undefined: Winsize
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/ioctl.go:61:42: undefined: Termios
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/syscall_openbsd.go:34:6: missing function body
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/syscall_unix_gc.go:12:6: missing function body
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/syscall_unix_gc.go:13:6: missing function body
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/syscall_unix_gc.go:14:6: missing function body
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/syscall_unix_gc.go:15:6: missing function body
    Error: ../../../go/pkg/mod/golang.org/x/sys@v0.0.0-20191026070338-33540a1f6037/unix/ioctl.go:61:42: too many errors

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

* CI: use GitHub Actions

Use GitHub actions to run golang-ci-lint, cross, and test.

The "Test()" target in Mage was a plain "go test -v ./...", and should be
portable to other CI systems if needed; running it through the mage file
effectively resulted in "compile a go binary to run go".

The "Lint()" target in Mage relied on Travis CI setting up Golang-CI lint
before it was executed, which required bash. Moving it to GitHub actions
also allowed it to be run on Windows. Golang CI can still be run locally
either through the mage file (which is kept for now), or running
`golangci-lint run ./...` after installing golangci-lint.

The "CrossBuild() Mage target is still used to perform cross-compile, as it
contains code to generate the GOOS/GOARCH matrix, which makes it easier
to run locally.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

* improve documentation about timestamp format

* update changelog

* go.mod: github.com/stretchr/testify v1.7.0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

* Change godoc badge to pkg.go.dev badge

* Add support for the logger private buffer pool.

* fix race condition for SetFormatter and properly fix SetReportCaller race as well

* Update README.md

* ci: add go 1.17 to test matrix

* bump golang.org/x/sys depency version

* indicates issues as stale automatically

* ci: run only on go 1.17

* reduce the list of cross build target

* remove duplicated build constraints line

* do not run the linter on windows

* Improve Log methods documentation

* bump version of golang.org/x/sys dependency

fixes sirupsen#1332

* bump version of golangci-lint

* update gopkg.in/yaml.v3 to v3.0.1

* Use text when shows the logrus output

* update dependencies

* Fix data race in hooks.test package

* Add instructions to use different log levels for local and syslog

This commit adds instructions to the syslog readme about how to
send different log levels to local logging (`log.SetLevel`) and
syslog hook.

fixes sirupsen#1369

* This commit fixes a potential denial of service vulnerability in logrus.Writer() that could be triggered by logging text longer than 64kb without newlines. Previously, the bufio.Scanner used by Writer() would hang indefinitely when reading such text without newlines, causing the application to become unresponsive.

* Scan text in 64KB chunks

This commit fixes a potential denial of service
vulnerability in logrus.Writer() that could be
triggered by logging text longer than 64KB
without newlines. Previously, the bufio.Scanner
used by Writer() would hang indefinitely when
reading such text without newlines, causing the
application to become unresponsive.

* Revert "Merge pull request sirupsen#1376 from ozfive/master"

This reverts commit 6acd903, reversing
changes made to e59b167.

* Revert "Revert "Merge pull request sirupsen#1376 from ozfive/master""

This reverts commit 352781d.

* fix panic in Writer

Commit 766cfec introduced this bug by defining an incorrect split
function. First it breaks the old behavior because it never splits at
newlines now. Second, it causes a panic because it never tells the
scanner to stop. See the bufio.ScanLines function, something like:
```
if atEOF && len(data) == 0 {
	return 0, nil, nil
}
```
is needed to do that.

This commit fixes it by restoring the old behavior and calling
bufio.ScanLines but also keep the 64KB check in place to avoid buffering
for to long.

Two tests are added to ensure it is working as expected.

Fixes sirupsen#1383

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

---------

Signed-off-by: CodeLingo Bot <bot@codelingo.io>
Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Co-authored-by: Albert Nigmatzianov <albertnigma@gmail.com>
Co-authored-by: CodeLingo Bot <bot@codelingo.io>
Co-authored-by: David Bariod <davidriod@googlemail.com>
Co-authored-by: georlav <georlav@gmail.com>
Co-authored-by: Sébastien Lavoie <lavoiesl@users.noreply.github.com>
Co-authored-by: Adam Renberg Tamm <adam.renbergtamm@shopify.com>
Co-authored-by: Adam Renberg Tamm <tgwizard@gmail.com>
Co-authored-by: Kirill Motkov <motkov.kirill@gmail.com>
Co-authored-by: tbunyk <tbunyk@gmail.com>
Co-authored-by: Emil Hessman <emil@hessman.se>
Co-authored-by: Andrey Tcherepanov <xwa8wnqa3a@snkmail.com>
Co-authored-by: Jessica Paczuski <jessica@publicsonar.com>
Co-authored-by: Haoran Xu <xuhr@seagroup.com>
Co-authored-by: Clément Chigot <clement.chigot@atos.net>
Co-authored-by: A. F <hello@clivern.com>
Co-authored-by: Nicolas Lepage <19571875+nlepage@users.noreply.github.com>
Co-authored-by: Lynn Cyrin <lynn@textio.com>
Co-authored-by: Lynn Cyrin <lynncyrin@gmail.com>
Co-authored-by: Christian Muehlhaeuser <muesli@gmail.com>
Co-authored-by: Simon Eskildsen <sirup@sirupsen.com>
Co-authored-by: Edward Muller <emuller@salesforce.com>
Co-authored-by: Jonathan Hall <flimzy@flimzy.com>
Co-authored-by: zxc <zhaoxuanchao@kids-creative.com.cn>
Co-authored-by: Joel Williams <flowonyx@gmail.com>
Co-authored-by: Pantelis Sampaziotis <psampaz@gmail.com>
Co-authored-by: Edward Muller <edwardam@interlix.com>
Co-authored-by: Edward Muller <freeformz@gmail.com>
Co-authored-by: lwsanty <lwsanty@gmail.com>
Co-authored-by: David Bariod <david.bariod@qonto.eu>
Co-authored-by: Taylor Wrobel <taywrobel@github.com>
Co-authored-by: Dmitri Goutnik <dg@syrec.org>
Co-authored-by: Alex Shi <hlcfan.yan@gmail.com>
Co-authored-by: nolleh <nolleh7707@gmail.com>
Co-authored-by: Mikolaj Holysz <miki123211@gmail.com>
Co-authored-by: Mark Phelps <markphelps@github.com>
Co-authored-by: Mark Phelps <mark.aaron.phelps@gmail.com>
Co-authored-by: Fabrizio Cirelli <cirelli94@gmail.com>
Co-authored-by: Deep Datta <deepdattax@gmail.com>
Co-authored-by: David Raleigh <davidraleigh@gmail.com>
Co-authored-by: Alisdair MacLeod <git@alisdairmacleod.co.uk>
Co-authored-by: Ariel Simulevski <ariel@simulevski.at>
Co-authored-by: Thomas Lacroix <thomas.lacroix@teads.tv>
Co-authored-by: ialidzhikov <i.alidjikov@gmail.com>
Co-authored-by: Tobias Klauser <tklauser@distanz.ch>
Co-authored-by: Sohel <thesohelsheikh@icloud.com>
Co-authored-by: Ichinose Shogo <shogo82148@gmail.com>
Co-authored-by: CreativeCactus <12624092+CreativeCactus@users.noreply.github.com>
Co-authored-by: David Bariod <dbariod@scaleway.com>
Co-authored-by: l-lindsay <llindsay@ca.ibm.com>
Co-authored-by: Alec Benzer <alec@level.com>
Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Co-authored-by: Billy Zaelani Malik <m.billyzaelani@gmail.com>
Co-authored-by: Qingshan Luo <edoger@qq.com>
Co-authored-by: Ruben de Vries <ruben@rubensayshi.com>
Co-authored-by: heui <runphp@qq.com>
Co-authored-by: anajavi <anajavi@users.noreply.github.com>
Co-authored-by: Nathan Johnson <nathan@nathanjohnson.org>
Co-authored-by: izhakmo <izhakmo@post.bgu.ac.il>
Co-authored-by: Griffin Abner <xieyuschen@gmail.com>
Co-authored-by: David Bariod <dbariod@synthesio.com>
Co-authored-by: Francois <wfrancoi@cisco.com>
Co-authored-by: Tommaso Visconti <tommaso.visconti@gmail.com>
Co-authored-by: Chris <straight.chris@gmail.com>
Co-authored-by: Paul Holzinger <pholzing@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants