Please include support for zerolog Panic (nil pointer check)

[eraserix]

Please add support for zerolog "Panic" to avoid triggering "SA5011: possible nil pointer dereference".

[dlsrb6342]

Hi @dominikh
Is there any update on this issue?

[ainar-g]

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]

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]

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

[arp242]

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]

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]

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]

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]

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.