Issue found by Svace static analyzer

[vvdvortsova]

Hello
This warning found with the Svace.
Pointer 'resp' returned from function 'concurrency.stm.fetch' at stm.go:316 may be null, and it is dereferenced at stm.go:320.

etcd/client/v3/concurrency/stm.go

Line 316 in 3f639e5

resp := s.stm.fetch(keys...)

Here resp may be dereferenced here.

etcd/client/v3/concurrency/stm.go

Line 320 in 3f639e5

v3.WithRev(resp.Header.Revision),

The function fetch returns nil value here.

etcd/client/v3/concurrency/stm.go

Line 278 in 3f639e5

return nil

[jmhbnz]

Hey @ahrtr , @serathius - While doing some exploration I found this issue which is our oldest open and un-triaged issue.

Reviewing main my naive interpretation is that this looks like a valid issue as resp can be used while possibly being nil. So a fix could be as simple as checking for nil before using resp and raising some kind of error or panic if nil so at least it is maintainable moving forward.

Can I please get your thoughts on this? If you think it's valid I will assign and propose a fix. Otherwise let's close this down as it has been open a long time.

[ahrtr]

Thanks both and sorry for the late response.

The passed keys will not be empty (len(keys) == 0), it will not run into any issue. We have four options,

1. Keep it as it's, because it actually will not run into any issue;
2. Change line 317 if firstRead to if firstRead && resp != nil && resp.Header != nil;
3. Add a check something like if len(keys) == 0 { return "" } at the beginning of the method; (preferred)
4. Add a assertion something like verify.Assert(len(keys) != 0, "no key provided");

[ShivangiM]

I would like to work on this issue, will raise pr for same

[jmhbnz]

Thanks @ShivangiM - I've assigned this issue to you.

[jmhbnz]

Hey @ShivangiM - How are you going with this? Please let us know if you have any questions 🙏🏻

[vianamjr]

I can do it, if it's still open.

[jmhbnz]

Closing as completed in #16198 - thanks @vianamjr 🙏🏻