Hey folks, another one ready for scaffold :raised_...
# atmos-dev
j
Hey folks, another one ready for scaffold šŸ™Œ github.com/cloudposse/atmos/pull/3230
šŸ‘€ 1
a
@Jorrit Elfferich thanks for the PR, there are comments/conversations - please review and address
j
Will tend to them sometime this week šŸ‘
Updated it, now shares the yaml tree walker.
Hm, some failing checks: • lint šŸ‘‰ rate limitting šŸ˜• • CodeQL šŸ‘‰ points to things this PR didn't touch, think this has to do with the merge commits or some timing issue with CodeQL creating a baseline or something? 2 annotations are on files I didn't even touch. 3 annotations are on generator/ui/ui.go but even in the review pane you can see they are made outside of patch/diff.
a
@Jorrit Elfferich please address the comments in the PRs
j
Can not access github.com/cloudposse/atmos/…/6287, but I took a stab at the overflow concern.
šŸ‘ 1
a
Copy code
Size computation for allocation may overflow #6287
Open
In pull request refs/pull/3230/merge • 3 days ago
Code snippet
pkg/generator/engine/matrix.go:339
		next := make([]map[string]string, 0, len(rows))
		for _, row := range rows {
			for _, value := range matrix[axis] {
				copied := make(map[string]string, len(row)+1)
 Warning
Size computation for allocation may overflow
This operation, which is used in an allocation
, involves a potentially large value
 and might overflow.
CodeQL
				for k, v := range row {
					copied[k] = v
				}

Description
Performing calculations involving the size of potentially large strings or slices can result in an overflow (for signed integer types) or a wraparound (for unsigned types). An overflow causes the result of the calculation to become negative, while a wraparound results in a small (positive) number.

This can cause further issues. If, for example, the result is then used in an allocation, it will cause a runtime panic if it is negative, and allocate an unexpectedly small buffer otherwise.

Recommendation
Always guard against overflow in arithmetic operations involving potentially large numbers by doing one of the following:

Validate the size of the data from which the numbers are computed.
Define a guard on the arithmetic expression, so that the operation is performed only if the result can be known to be less than, or equal to, the maximum value for the type.
Use a wider type (such as uint64 instead of int), so that larger input values do not cause overflow.
Example
In the following example, assume that there is a function encryptBuffer that encrypts byte slices whose length must be padded to be a multiple of 16. The function encryptValue provides a convenience wrapper around this function: when passed an arbitrary value, it first encodes that value as JSON, pads the resulting byte slice, and then passes it to encryptBuffer.

package main

import "encoding/json"

func encryptValue(v interface{}) ([]byte, error) {
	jsonData, err := json.Marshal(v)
	if err != nil {
		return nil, err
	}
	size := len(jsonData) + (len(jsonData) % 16)
	buffer := make([]byte, size)
	copy(buffer, jsonData)
	return encryptBuffer(buffer)
}
When passed a value whose JSON encoding is close to the maximum value of type int in length, the computation of size will overflow, producing a negative value. When that negative value is passed to make, a runtime panic will occur.

To guard against this, the function should be improved to check the length of the JSON-encoded value. For example, here is a version of encryptValue that ensures the value is no larger than 64 MB, which fits comfortably within an int and avoids the overflow:

package main

import (
	"encoding/json"
	"errors"
)

func encryptValueGood(v interface{}) ([]byte, error) {
	jsonData, err := json.Marshal(v)
	if err != nil {
		return nil, err
	}
	if len(jsonData) > 64*1024*1024 {
		return nil, errors.New("value too large")
	}
	size := len(jsonData) + (len(jsonData) % 16)
	buffer := make([]byte, size)
	copy(buffer, jsonData)
	return encryptBuffer(buffer)
}
References
The Go Programming Language Specification: Integer overflow.
The Go Programming Language Specification: Making slices, maps and channels.
Common Weakness Enumeration: CWE-190.
j
Yeah, makes sense. Implemented it with a guard, same like the
encryptValueGood
example where it clamps to a max
a
@Jorrit Elfferich thanks again for all your effort, please let me know when to re-review the two PRs
j
No worries šŸ™‡. They are both ready, but will conflict one another. I'll leave it for you to decide which one can be merged first šŸ™‚.
I am obviously happy to resolve the conflicts when one does get merged, no problem!
šŸ‘ 1
I guess CodeQL made a decision šŸ˜…
a
this one is merged github.com/cloudposse/atmos/pull/3245 - need to wait for the merge queue.
šŸ™Œ 1
this one will require some work (commants + conflicts after the first one merges): github.com/cloudposse/atmos/pull/3230
j
Yeah, I also saw @Erik Osterman (Cloud Posse) brought support for another YAML function to atmos.yaml files (
!literal
function if im not mistaken)? My branch might collide with the same files.. I will figure it out, buff it up, and let you know when ready šŸ™‚
šŸ‘ 1
Ready. I see github.com/cloudposse/atmos/pull/3275 is not yet merged, also a stacked PR so not sure what its ETA is. But it will def collide on
pkg/utils/yaml_utils.go
. Let me know how I can accomodate here šŸ™.
e
That's fine - merge conflicts are no longer a PIA
We will merge yours first probably
j
Driving conflicts through ai is indeed a blessing šŸ˜„
šŸ’Æ 1
šŸ‘ 1