gosec

[lburgazzoli]

Release Note

NONE

[lburgazzoli]

@astefanutti @squakez I've done a quick scanning with gosec by enabling the related lint in golangci-lint (see #2763).

I'm not sure if the change I made are the proper one but the intention is more to make them a little bit visible so we can agree on the path we want to follow in general.

[astefanutti]

Great!

[lburgazzoli]

@astefanutti @squakez I've done a quick scanning with gosec by enabling the related lint in golangci-lint (see #2763).

I'm not sure if the change I made are the proper one but the intention is more to make them a little bit visible so we can agree on the path we want to follow in general.

As example one of the reported issue is about Implicit memory aliasing in for loop which may lead to very bad results but looking at the code, it should not happen because the methods that re invoked are stateless but still the caller should not make assumption thus is better to fix it.

[squakez]

Excellent job! it's going to be a very helpful tool!

[lburgazzoli]

@astefanutti @squakez

I've somehow fixed some of the issues reported by golanci-lint and gosec, however I need some feedback for the fixes in particular for this commit where I've tried to address deferring invocation of "Close" on type "*os.File", like:

f, err := os.Open("/foo/bar.txt")
if err != nil {
    return err
}
defer f.Close()

The reason of this finding is that, when deferring the method call, we don't check if the file is really closed or not and in most of the case it is probably ok but it may cause very subtle bugs like we may leak file descriptor that may or may not impact the behavior of the application at some point (i.e. you may run out of file descriptors). So I think that when possible this problem should be addressed.

However it is not very clear how to implement it. Some hint may be found in this blog post: https://www.joeshaw.org/dont-defer-close-on-writable-files.

I've tried to implement what is written in that post but it does not seem to be applicable as a general rule so I ended up with some slightly different way (that also leverage some of the hints from the blog) that make use of closures. I'm not sure if that is the right approach or not so any feedback would be more that welcome.

[lburgazzoli]

gosec action is not allowed, I've raised an issue: https://issues.apache.org/jira/browse/INFRA-22558

[astefanutti]

@lburgazzoli the closure pattern looks good.

To be honest, even the "plain procedural" approach looks good to me:

f, err := os.Create("path")
if err != nil {
    return err
}

if err = io.WriteString(f, "hello world"); err != nil {
    f.Close()
    return err
}

return f.Close()

Sometimes, no-pattern is the best pattern, and I find it a bit ironic the hype with defer biased us to use it brokenly, in contradiction with the general Golang philosophy, that aims for minimalism and robustness, like with error handling for example.

[lburgazzoli]

@lburgazzoli the closure pattern looks good.

To be honest, even the "plain procedural" approach looks good to me:

I do agree on this point and I originally opted to use it, however there is a subtle issue in this block:

if err = io.WriteString(f, "hello world"); err != nil {
    f.Close()
    return err
}

Is it correct to swallow any error that may be detected while closing the file ? In my case I experimented with the multierr package to combine multiple errors but that's also something that I'm not sure about.

I guess here what we need to find is the logical pattern we want to use to deal with this case and enforce with gosec/lint and when doing PR review rather than the specific coding patter (like the closure I introduced as exploration).

Sometimes, no-pattern is the best pattern, and I find it a bit ironic the hype with defer biased us to use it brokenly, in contradiction with the general Golang philosophy, that aims for minimalism and robustness, like with error handling for example.

100% agree

[astefanutti]

@lburgazzoli the closure pattern looks good.
To be honest, even the "plain procedural" approach looks good to me:

I do agree on this point and I originally opted to use it, however there is a subtle issue in this block:

if err = io.WriteString(f, "hello world"); err != nil {
    f.Close()
    return err
}

Is it correct to swallow any error that may be detected while closing the file ? In my case I experimented with the multierr package to combine multiple errors but that's also something that I'm not sure about.

The author of the blog post indicates that the close error is ignored, because the write error takes precedence. I'm not sure how to interpret that, but I guess most importantly, an error is returned, and the caller error handling flow is triggered. Also close is called, whatever the result is, but an error is still returned, providing details why the write fails. Trying to combine details about the write and close errors is possible, but I'm not sure that's worth the extra complexity.

[lburgazzoli]

@lburgazzoli the closure pattern looks good.

Is it correct to swallow any error that may be detected while closing the file ? In my case I experimented with the multierr package to combine multiple errors but that's also something that I'm not sure about.

The author of the blog post indicates that the close error is ignored, because the write error takes precedence. I'm not sure how to interpret that, but I guess most importantly, an error is returned, and the caller error handling flow is triggered.

I'm not really convinced about the wording used in the blog as the fact that write error takes precedence over the close error is an opinionated choice as we are talking about errors that may or may not be related (got bitten hard by a similar assumption in the past in the java land, so I'm a little bit sensitive about the problem).

However I do agree that at least the caller is aware of an error.
Let me try to get the build succeed (may need some help) and I'll try to explore if the process can me made simpler

[astefanutti]

The author of the blog post indicates that the close error is ignored, because the write error takes precedence. I'm not sure how to interpret that, but I guess most importantly, an error is returned, and the caller error handling flow is triggered.

I'm not really convinced about the wording used in the blog as the fact that write error takes precedence over the close error is an opinionated choice as we are talking about errors that may or may not be related (got bitten hard by a similar assumption in the past in the java land, so I'm a little bit sensitive about the problem).

I agree it's hard to tell whether that's an opinion or a technical argument.

Let me try to get the build succeed (may need some help) and I'll try to explore if the process can me made simpler

Sounds good.

[tadayosi]

I changed in a previous commit here from enable-all (denylisting) to disable-all (allowlisting), because otherwise every time we try to upgrade golangci-lint it will introduce a new set of linters which weren't checked before and thus upgrading golangci-lint will become painful. I thought using disable-all and allowlisting makes upgrading the golangci-lint action easier.

So, do you think it's not a good idea and we should catch up with new additions to linters along with upgrade of the action?

[lburgazzoli]

I can revert to the disable-all mode, I don't have any strong opinion here.

The reason I did it the other way is to be sure we are not missing any important lint (as example. the gosec one was missing) so I did try to run it with all the possible lints and then disabling those that are just noise.

@astefanutti @squakez @tadayosi since I'm not contributing to the project very often now, I let you decide

[astefanutti]

On one hand with disable-all, it's less work to upgrade, but we would lag behind what would be the "default" / de-facto standard set of linters, and we would create a "debt" over time that would have to be pay by some larger one-time efforts, on the other hand with enable-all, there is some extra work possible each time we upgrade, which may biased us from not upgrading, but we would inherit from the "default", possibly better set of linters.

I'd be inclined toward trusting golangci-lint and trying with enable-all, so we dilute the possible overhead, keeping in mind that we can always revert to disable-all if that does not work.

[lburgazzoli]

can we merge this PR then ?

[astefanutti]

I think so, was waiting for your final confirmation :)

[tadayosi]

Fair enough. Let's use enable-all and see how it works.