You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/11/22 12:50:53 UTC

[GitHub] [camel-k] lburgazzoli opened a new pull request #2766: gosec

lburgazzoli opened a new pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766


   - fix(gosec): Implicit memory aliasing in for loop (G601)
   - fix(gosec): Expect WriteFile permissions to be 0600 or less (G306)
   - fix(gosec): Use of weak random number generator (G404)
   - fix(gosec): disable rule G101
   - chore(ci): update golangci-lint to v1.43
   
   <!-- Description -->
   
   
   
   
   <!--
   Enter your extended release note in the below block. If the PR requires
   additional action from users switching to the new release, include the string
   "action required". If no release note is required, write "NONE". 
   
   You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
   the text is added to the right section of the release notes. 
   -->
   
   **Release Note**
   ```release-note
   NONE
   ```
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976009518


   @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](https://github.com/apache/camel-k/pull/2766/commits/0a5b577a1a423db5c58019f98acd88afdb685998) commit where I've tried to address deferring invocation of "Close" on type "*os.File", like:
   
   ```go
   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 (it looks like the standard library is doing so) 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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on a change in pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#discussion_r756033753



##########
File path: .golangci.yml
##########
@@ -18,54 +18,46 @@
 linters-settings:
   lll:
     line-length: 170
+  goconst:
+    ignore-tests: true
 linters:
-  disable-all: true
-  enable:
-    - asciicheck
-    - bodyclose
-    - deadcode
-    - depguard
-    - dogsled
-    - durationcheck
-    - errcheck
-    - errname
-    - errorlint
-    - exportloopref
-    - forcetypeassert
-    - gocritic
-    - gofmt
+  enable-all: true

Review comment:
       can we merge this PR then ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976014782


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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-975494300


   @astefanutti @squakez I've done a quick scanning with `gosec` by enabling the related lint in `golangci-lint` (see https://github.com/apache/camel-k/issues/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] tadayosi commented on a change in pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
tadayosi commented on a change in pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#discussion_r755685485



##########
File path: .golangci.yml
##########
@@ -18,54 +18,46 @@
 linters-settings:
   lll:
     line-length: 170
+  goconst:
+    ignore-tests: true
 linters:
-  disable-all: true
-  enable:
-    - asciicheck
-    - bodyclose
-    - deadcode
-    - depguard
-    - dogsled
-    - durationcheck
-    - errcheck
-    - errname
-    - errorlint
-    - exportloopref
-    - forcetypeassert
-    - gocritic
-    - gofmt
+  enable-all: true

Review comment:
       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?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti edited a comment on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti edited a comment on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976237140


   @lburgazzoli the closure pattern looks good.
   
   To be honest, even the "plain procedural" approach looks good to me:
   
   ```go
   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. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] tadayosi commented on a change in pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
tadayosi commented on a change in pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#discussion_r756537107



##########
File path: .golangci.yml
##########
@@ -18,54 +18,46 @@
 linters-settings:
   lll:
     line-length: 170
+  goconst:
+    ignore-tests: true
 linters:
-  disable-all: true
-  enable:
-    - asciicheck
-    - bodyclose
-    - deadcode
-    - depguard
-    - dogsled
-    - durationcheck
-    - errcheck
-    - errname
-    - errorlint
-    - exportloopref
-    - forcetypeassert
-    - gocritic
-    - gofmt
+  enable-all: true

Review comment:
       Fair enough. Let's use `enable-all` and see how it works.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti commented on a change in pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#discussion_r755779181



##########
File path: .golangci.yml
##########
@@ -18,54 +18,46 @@
 linters-settings:
   lll:
     line-length: 170
+  goconst:
+    ignore-tests: true
 linters:
-  disable-all: true
-  enable:
-    - asciicheck
-    - bodyclose
-    - deadcode
-    - depguard
-    - dogsled
-    - durationcheck
-    - errcheck
-    - errname
-    - errorlint
-    - exportloopref
-    - forcetypeassert
-    - gocritic
-    - gofmt
+  enable-all: true

Review comment:
       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.
    




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti edited a comment on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti edited a comment on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976237140


   @lburgazzoli the closure pattern looks good.
   
   To be honest, even the "plain procedural" approach looks good to me:
   
   ```go
   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 use to use it brokenly, in contradiction with the general Golang philosophy, that aims for minimalism and robustness, like with error handling for example. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on a change in pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on a change in pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#discussion_r755737923



##########
File path: .golangci.yml
##########
@@ -18,54 +18,46 @@
 linters-settings:
   lll:
     line-length: 170
+  goconst:
+    ignore-tests: true
 linters:
-  disable-all: true
-  enable:
-    - asciicheck
-    - bodyclose
-    - deadcode
-    - depguard
-    - dogsled
-    - durationcheck
-    - errcheck
-    - errname
-    - errorlint
-    - exportloopref
-    - forcetypeassert
-    - gocritic
-    - gofmt
+  enable-all: true

Review comment:
       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




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976399960


   > > 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 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976247107


   > @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: 
   
   ```go
   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 use 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
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti edited a comment on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti edited a comment on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976399960


   > > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti merged pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti merged pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti commented on a change in pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#discussion_r756035270



##########
File path: .golangci.yml
##########
@@ -18,54 +18,46 @@
 linters-settings:
   lll:
     line-length: 170
+  goconst:
+    ignore-tests: true
 linters:
-  disable-all: true
-  enable:
-    - asciicheck
-    - bodyclose
-    - deadcode
-    - depguard
-    - dogsled
-    - durationcheck
-    - errcheck
-    - errname
-    - errorlint
-    - exportloopref
-    - forcetypeassert
-    - gocritic
-    - gofmt
+  enable-all: true

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




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976270481


   > > @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:
   > 
   > ```go
   > 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.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] astefanutti commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
astefanutti commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976237140


   @lburgazzoli the closure pattern looks good.
   
   To be honest, even the "plain procedural" approach looks good to me:
   
   ```go
   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 use to use it brokenly, in contradiction with the general Goland philosophy, that aims for minimalism and robustness, like with error handling for example. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976294285


   > > > @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
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli edited a comment on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli edited a comment on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976247107


   > @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: 
   
   ```go
   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
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli edited a comment on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli edited a comment on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-976009518


   @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](https://github.com/apache/camel-k/pull/2766/commits/0a5b577a1a423db5c58019f98acd88afdb685998) commit where I've tried to address deferring invocation of "Close" on type "*os.File", like:
   
   ```go
   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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli commented on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-975491973


   @astefanutti @squakez I've done a quick scanning with `gosec` by enabling the related lint in `golangci-lint` (see https://github.com/apache/camel-k/issues/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [camel-k] lburgazzoli edited a comment on pull request #2766: gosec

Posted by GitBox <gi...@apache.org>.
lburgazzoli edited a comment on pull request #2766:
URL: https://github.com/apache/camel-k/pull/2766#issuecomment-975494300


   @astefanutti @squakez I've done a quick scanning with `gosec` by enabling the related lint in `golangci-lint` (see https://github.com/apache/camel-k/issues/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. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org