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.

Sign up for GitHub

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

Jump to bottom

Please include support for zerolog Panic (nil pointer check) #793

Open
eraserix opened this issue Jul 2, 2020 · 9 comments · May be fixed by #1551
Open

Please include support for zerolog Panic (nil pointer check) #793

eraserix opened this issue Jul 2, 2020 · 9 comments · May be fixed by #1551
Labels
false-positive

Comments

@eraserix
Copy link

eraserix commented Jul 2, 2020

Please add support for zerolog "Panic" to avoid triggering "SA5011: possible nil pointer dereference".
@eraserix eraserix added false-positive needs-triage Newly filed issue that needs triage labels Jul 2, 2020
@dominikh dominikh removed the needs-triage Newly filed issue that needs triage label Oct 4, 2020
@dlsrb6342
Copy link

Hi @dominikh
Is there any update on this issue?

@ainar-g
Copy link
Contributor

ainar-g commented Feb 11, 2022

I think that that was discussed somewhere else, but perhaps a list of fully-qualified identifier names of terminating functions could be added to staticcheck.conf. So that people could update the for their projects' needs instead of teaching staticcheck about all logging and utility libraries.

@dlsrb6342
Copy link

Please file an issue if we’re missing support for a popular package.

I found this in docs.

go-tools/ir/exits.go

Lines 22 to 60 in 82c7dc9

case "github.com/sirupsen/logrus":
switch obj.(*types.Func).FullName() {
case "(*github.com/sirupsen/logrus.Logger).Exit":
// Technically, this method does not unconditionally exit
// the process. It dynamically calls a function stored in
// the logger. If the function is nil, it defaults to
// os.Exit.
//
// The main intent of this method is to terminate the
// process, and that's what the vast majority of people
// will use it for. We'll happily accept some false
// negatives to avoid a lot of false positives.
fn.WillExit = true
return
case "(*github.com/sirupsen/logrus.Logger).Panic",
"(*github.com/sirupsen/logrus.Logger).Panicf",
"(*github.com/sirupsen/logrus.Logger).Panicln":
// These methods will always panic, but that's not
// statically known from the code alone, because they
// take a detour through the generic Log methods.
fn.WillUnwind = true
return
case "(*github.com/sirupsen/logrus.Entry).Panicf",
"(*github.com/sirupsen/logrus.Entry).Panicln":
// Entry.Panic has an explicit panic, but Panicf and
// Panicln do not, relying fully on the generic Log
// method.
fn.WillUnwind = true
return
case "(*github.com/sirupsen/logrus.Logger).Log",
"(*github.com/sirupsen/logrus.Logger).Logf",
"(*github.com/sirupsen/logrus.Logger).Logln":
// TODO(dh): we cannot handle these case. Whether they
// exit or unwind depends on the level, which is set
// via the first argument. We don't currently support
// call-site-specific exit information.
}

And staticcheck only supports logrus. I think zerolog can be added.

@ainar-g Could you give guide how to add terminating functions to configuration?

@ainar-g
Copy link
Contributor

ainar-g commented Feb 14, 2022

@dlsrb6342, I cannot provide any guides, since the feature doesn't exist yet. My comment was merely a proposal to add it.

@arp242
Copy link
Contributor

arp242 commented May 27, 2024

Prototype for adding a new unreachables config option: 5743549

[unreachables]
'(*github.com/rs/zerolog.Logger).Fatal' = false
'(*github.com/rs/zerolog.Logger).Panic' = true

The value indicates if the fn.NoReturn = irAlwaysUnwinds (rather than ir.AlwaysExits).

Few bits need to be done, and should move that switch to the default config, but seems to work.

Any feedback on this approach @dominikh before I work on this further?

@dominikh
Copy link
Owner

I don't think I want to make this configurable. This information affects how the IR gets built, and incorrect settings could lead to incorrect IR and hard to diagnose problems.

@arp242
Copy link
Contributor

arp242 commented May 27, 2024 •
edited
Loading

Can just add zerolog to that list I guess, but would be nice to configure this. I don't see a huge potential for failure here, as it checks functions it finds against the configuration (rather than using the config as direct input). So the worst is you end up with a fatal function that's not fatal. Some junk values is not going to cause Weird Stuff™ to happen.

@dominikh
Copy link
Owner

So the worst is you end up with a fatal function that's not fatal

Which affects all IR-based analyzes that try to reason about the control flow of code after a non-fatal fatal function gets called.

arp242 added a commit to arp242/go-tools that referenced this issue May 29, 2024
@arp242
go/ir: mark zerolog functions as exit/panic
96dcd82 
Fixes dominikh#793
@arp242 arp242 linked a pull request May 29, 2024 that will close this issue
Open
@arp242
Copy link
Contributor

arp242 commented May 29, 2024

It's just that I don't see how someone could accidentally get this wrong by a typo or misunderstanding. Personally I would say that adding non-exiting functions here fits in the "that's just stupid, don't do that"-category.

But no worries, it hasn't come up that often so probably not that big of a deal. Although with e.g. #1526 it might be a bit more of a deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
false-positive
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

go/ir: mark zerolog functions as exit/panic arp242/go-tools
5 participants
@dominikh @arp242 @ainar-g @eraserix @dlsrb6342