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.

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

False positive when a loop var is exported, directly followed by break #14

Closed
alvaroaleman opened this issue Jul 16, 2021 · 5 comments
Closed
Assignees
Labels
help wanted Extra attention is needed

Comments

@alvaroaleman
Copy link

exportloopref complains about something like this, even though it is valid:

var result *int
haystack := []int{1, 2}
for _, hay := range haystack {
 if true {
   result = &hay
   break
  }
}
@kyoh86
Copy link
Owner

kyoh86 commented Jul 31, 2021

📝 to generalize

💭 What kind of structure is a alignment directly followed by a break?

result = &hay
(...something no mattered...)
break

NOTE: no mattered equals no condition, no continue.

-> there's no problem.

no problem cases

directly followed by break:

result = &p
break-

there's steps that never continue:

result = &p
print(result)
break

breaks on parent branch:

if <cond.1> {
    if <cond.2> {
        result = &p
    }
    break
}

diagnostics

breaks sometimes

result = &p
if <cond> {
    break
}
result = &p
...
if <cond> {
    continue // it will ignore the breaking branch.
}
...
break

Cost to probe the diagnostics

  • Easy to probe
    • Which loop will be broken.
  • Hard to probe (high cost)
    • There's break after the alignment.
    • There's no condition (if) between the alignment and break.
    • There's no continue between the alignment and break.

How to do

  • First pass, find the exporting alignments as candidates.
  • Second pass, check whether they are followed by break or not.
  • Third pass, check whether continue exists before break.

Worry

Maybe to resolve this, exportloopref will be slow (about 3 times)
Even if we care just about the DIRECTLY followed by break (without any lines between the break and alignment), it needs 2 times cost.

@kyoh86
Copy link
Owner

kyoh86 commented Jul 31, 2021

To make it simple:

First pass

  • Store the candidates with if, for and switch stacks
  • Store breaks with if, for and switch stacks.
  • Store continues with parent loop.

Second pass

For each candidates:

  • Check whether there's any break which has same stack.
  • Check whether the loop has no continue.

@alvaroaleman
Copy link
Author

@kyoh86 thank you for looking into this! I would already be very happy if the case of "pointer assignment directly followed by break/return" would work.

@kyoh86
Copy link
Owner

kyoh86 commented Aug 2, 2021

Your requirement is so simple.
But I have not found a simple way to just do it.
I'm trying to look for it but also to implement complex pass that is, as shown above.

If you have an idea, please give me a pull request.

@kyoh86 kyoh86 added the help wanted Extra attention is needed label Nov 16, 2022
mtardy added a commit to cilium/tetragon that referenced this issue May 10, 2023
Exporting pointers for loop variables can create very hard to debug bug.
Contrary to what can think, the `:=` assignment in the range loop does
not create a new local variable on each iteration, see the following Go
Language specification extract.

"The iteration variables may be declared by the “range” clause using a
form of short variable declaration (:=). In this case their types are
set to the types of the respective iteration values and their scope is
the block of the "for" statement; they are re-used in each iteration. If
the iteration variables are declared outside the "for" statement, after
execution their values will be those of the last iteration."

Creating a local variable fix this issue and allocate new memory each
time making it possible to export the pointer.

When immediatly using break after, it's okay but linter is not capable
of detecting that because of perf slowdown, see kyoh86/exportloopref#14.

See more info:
- https://github.com/kyoh86/exportloopref
- https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
mtardy added a commit to cilium/tetragon that referenced this issue May 10, 2023
Exporting pointers for loop variables can create very hard to debug bug.
Contrary to what can think, the `:=` assignment in the range loop does
not create a new local variable on each iteration, see the following Go
Language specification extract.

"The iteration variables may be declared by the “range” clause using a
form of short variable declaration (:=). In this case their types are
set to the types of the respective iteration values and their scope is
the block of the "for" statement; they are re-used in each iteration. If
the iteration variables are declared outside the "for" statement, after
execution their values will be those of the last iteration."

Creating a local variable fix this issue and allocate new memory each
time making it possible to export the pointer.

When immediately using break after, it's okay but linter is not capable
of detecting that because of perf slowdown, see kyoh86/exportloopref#14.

See more info:
- https://github.com/kyoh86/exportloopref
- https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
mtardy added a commit to cilium/tetragon that referenced this issue May 11, 2023
Exporting pointers for loop variables can create very hard to debug bug.
Contrary to what can think, the `:=` assignment in the range loop does
not create a new local variable on each iteration, see the following Go
Language specification extract.

"The iteration variables may be declared by the “range” clause using a
form of short variable declaration (:=). In this case their types are
set to the types of the respective iteration values and their scope is
the block of the "for" statement; they are re-used in each iteration. If
the iteration variables are declared outside the "for" statement, after
execution their values will be those of the last iteration."

Creating a local variable fix this issue and allocate new memory each
time making it possible to export the pointer.

When immediately using break after, it's okay but linter is not capable
of detecting that because of perf slowdown, see kyoh86/exportloopref#14.

See more info:
- https://github.com/kyoh86/exportloopref
- https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
mtardy added a commit to cilium/tetragon that referenced this issue May 11, 2023
Exporting pointers for loop variables can create very hard to debug bug.
Contrary to what can think, the `:=` assignment in the range loop does
not create a new local variable on each iteration, see the following Go
Language specification extract.

"The iteration variables may be declared by the “range” clause using a
form of short variable declaration (:=). In this case their types are
set to the types of the respective iteration values and their scope is
the block of the "for" statement; they are re-used in each iteration. If
the iteration variables are declared outside the "for" statement, after
execution their values will be those of the last iteration."

Creating a local variable fix this issue and allocate new memory each
time making it possible to export the pointer.

When immediately using break after, it's okay but linter is not capable
of detecting that because of perf slowdown, see kyoh86/exportloopref#14.

See more info:
- https://github.com/kyoh86/exportloopref
- https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
mtardy added a commit to cilium/tetragon that referenced this issue May 30, 2023
Exporting pointers for loop variables can create very hard to debug bug.
Contrary to what can think, the `:=` assignment in the range loop does
not create a new local variable on each iteration, see the following Go
Language specification extract.

"The iteration variables may be declared by the “range” clause using a
form of short variable declaration (:=). In this case their types are
set to the types of the respective iteration values and their scope is
the block of the "for" statement; they are re-used in each iteration. If
the iteration variables are declared outside the "for" statement, after
execution their values will be those of the last iteration."

Creating a local variable fix this issue and allocate new memory each
time making it possible to export the pointer.

When immediately using break after, it's okay but linter is not capable
of detecting that because of perf slowdown, see kyoh86/exportloopref#14.

See more info:
- https://github.com/kyoh86/exportloopref
- https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
@kyoh86
Copy link
Owner

kyoh86 commented May 19, 2024

In Go 1.22, it is not a problem

@kyoh86 kyoh86 closed this as not planned Won't fix, can't repro, duplicate, stale May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants