-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
helper
package functions to copy maps/slices encourage unsafe usage
#11564
Labels
Projects
Comments
tgross
added a commit
that referenced
this issue
Nov 24, 2021
In the system scheduler, if a subset of clients are filtered by class, we hit a code path where the `AllocMetric` has been copied, but the `Copy` method does not instantiate the various maps. This leads to an assignment to a nil map. This changeset ensures that the maps are non-nil before continuing. The `Copy` method relies on functions in the `helper` package that all return nil slices or maps when passed zero-length inputs. This changeset to fix the panic bug intentionally defers updating those functions because it'll have potential impact on memory usage. See #11564 for more details.
tgross
added a commit
that referenced
this issue
Nov 24, 2021
In the system scheduler, if a subset of clients are filtered by class, we hit a code path where the `AllocMetric` has been copied, but the `Copy` method does not instantiate the various maps. This leads to an assignment to a nil map. This changeset ensures that the maps are non-nil before continuing. The `Copy` method relies on functions in the `helper` package that all return nil slices or maps when passed zero-length inputs. This changeset to fix the panic bug intentionally defers updating those functions because it'll have potential impact on memory usage. See #11564 for more details.
tgross
added a commit
that referenced
this issue
Nov 24, 2021
) In the system scheduler, if a subset of clients are filtered by class, we hit a code path where the `AllocMetric` has been copied, but the `Copy` method does not instantiate the various maps. This leads to an assignment to a nil map. This changeset ensures that the maps are non-nil before continuing. The `Copy` method relies on functions in the `helper` package that all return nil slices or maps when passed zero-length inputs. This changeset to fix the panic bug intentionally defers updating those functions because it'll have potential impact on memory usage. See #11564 for more details.
lgfa29
pushed a commit
that referenced
this issue
Nov 24, 2021
) In the system scheduler, if a subset of clients are filtered by class, we hit a code path where the `AllocMetric` has been copied, but the `Copy` method does not instantiate the various maps. This leads to an assignment to a nil map. This changeset ensures that the maps are non-nil before continuing. The `Copy` method relies on functions in the `helper` package that all return nil slices or maps when passed zero-length inputs. This changeset to fix the panic bug intentionally defers updating those functions because it'll have potential impact on memory usage. See #11564 for more details.
Fixed by #14139 |
I'm going to lock this issue because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active issues. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The various generic copy helper functions in the
helper
package returnnil
if the input is zero-length. This was likely done to save memory by avoiding heap-allocating empty maps and slices (a few pointers each). But this encourages usage wherenil
maps are being passed around and need to be checked everywhere after a copy, or we can hit a panic. See #11563 (comment) for an example.The functions have a lot of callers, so one approach we might want to consider here is having the
Copy
methods in thenomad/structs
package use new variants that allocate the empty map.The text was updated successfully, but these errors were encountered: