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

bug: resolve type conversion alerts #20553

Merged
merged 7 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/20553.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
core: Fix multiple incorrect type conversion for potential overflows
```
3 changes: 3 additions & 0 deletions api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -1184,6 +1184,9 @@ func parseQueryMeta(resp *http.Response, q *QueryMeta) error {
if err != nil {
return fmt.Errorf("Failed to parse X-Nomad-LastContact: %v", err)
}
if last > uint64(math.MaxInt64) {
tgross marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("Last contact duration is out of range: %d", last)
}
q.LastContact = time.Duration(last) * time.Millisecond
q.NextToken = header.Get("X-Nomad-NextToken")

Expand Down
19 changes: 11 additions & 8 deletions client/lib/numalib/detect_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,10 @@ func (*Sysfs) discoverCosts(st *Topology, readerFunc pathReaderFn) {
}

for i, c := range strings.Fields(s) {
cost, _ := strconv.Atoi(c)
cost, _ := strconv.ParseUint(c, 10, 8)
if err != nil {
return err
}
tgross marked this conversation as resolved.
Show resolved Hide resolved
st.Distances[id][i] = Cost(cost)
}
return nil
Expand All @@ -110,8 +113,8 @@ func (*Sysfs) discoverCores(st *Topology, readerFunc pathReaderFn) {
st.NodeIDs = idset.From[hw.NodeID]([]hw.NodeID{0})
const node = 0
const socket = 0
cpuMax, _ := getNumeric[hw.KHz](cpuMaxFile, readerFunc, core)
base, _ := getNumeric[hw.KHz](cpuBaseFile, readerFunc, core)
cpuMax, _ := getNumeric[hw.KHz](cpuMaxFile, 64, readerFunc, core)
base, _ := getNumeric[hw.KHz](cpuBaseFile, 64, readerFunc, core)
st.insert(node, socket, core, Performance, cpuMax, base)
return nil
})
Expand All @@ -126,9 +129,9 @@ func (*Sysfs) discoverCores(st *Topology, readerFunc pathReaderFn) {
cores := idset.Parse[hw.CoreID](string(s))
_ = cores.ForEach(func(core hw.CoreID) error {
// best effort, zero values are defaults
socket, _ := getNumeric[hw.SocketID](cpuSocketFile, readerFunc, core)
cpuMax, _ := getNumeric[hw.KHz](cpuMaxFile, readerFunc, core)
base, _ := getNumeric[hw.KHz](cpuBaseFile, readerFunc, core)
socket, _ := getNumeric[hw.SocketID](cpuSocketFile, 8, readerFunc, core)
cpuMax, _ := getNumeric[hw.KHz](cpuMaxFile, 64, readerFunc, core)
base, _ := getNumeric[hw.KHz](cpuBaseFile, 64, readerFunc, core)
siblings, _ := getIDSet[hw.CoreID](cpuSiblingFile, readerFunc, core)

// if we get an incorrect core number, this means we're not getting the right
Expand All @@ -154,13 +157,13 @@ func getIDSet[T idset.ID](path string, readerFunc pathReaderFn, args ...any) (*i
return idset.Parse[T](string(s)), nil
}

func getNumeric[T int | idset.ID](path string, readerFunc pathReaderFn, args ...any) (T, error) {
func getNumeric[T int | idset.ID](path string, maxSize int, readerFunc pathReaderFn, args ...any) (T, error) {
path = fmt.Sprintf(path, args...)
s, err := readerFunc(path)
if err != nil {
return 0, err
}
i, err := strconv.Atoi(strings.TrimSpace(string(s)))
i, err := strconv.ParseUint(strings.TrimSpace(string(s)), 10, maxSize)
if err != nil {
return 0, err
}
Expand Down
29 changes: 20 additions & 9 deletions command/agent/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -903,21 +903,23 @@ func parseWait(resp http.ResponseWriter, req *http.Request, b *structs.QueryOpti
}

// parseConsistency is used to parse the ?stale query params.
func parseConsistency(resp http.ResponseWriter, req *http.Request, b *structs.QueryOptions) {
func parseConsistency(resp http.ResponseWriter, req *http.Request, b *structs.QueryOptions) error {
query := req.URL.Query()
if staleVal, ok := query["stale"]; ok {
if len(staleVal) == 0 || staleVal[0] == "" {
b.AllowStale = true
return
return nil
}
staleQuery, err := strconv.ParseBool(staleVal[0])
if err != nil {
errMsg := "Expect `true` or `false` for `stale` query string parameter"
resp.WriteHeader(http.StatusBadRequest)
_, _ = resp.Write([]byte(fmt.Sprintf("Expect `true` or `false` for `stale` query string parameter, got %s", staleVal[0])))
return
resp.Write([]byte(errMsg))
return CodedError(http.StatusBadRequest, errMsg)
tgross marked this conversation as resolved.
Show resolved Hide resolved
}
b.AllowStale = staleQuery
}
return nil
}

// parsePrefix is used to parse the ?prefix query param
Expand Down Expand Up @@ -1014,27 +1016,36 @@ func (s *HTTPServer) parseToken(req *http.Request, token *string) {
func (s *HTTPServer) parse(resp http.ResponseWriter, req *http.Request, r *string, b *structs.QueryOptions) bool {
s.parseRegion(req, r)
s.parseToken(req, &b.AuthToken)
parseConsistency(resp, req, b)
if err := parseConsistency(resp, req, b); err != nil {
return true
}
parsePrefix(req, b)
parseNamespace(req, &b.Namespace)
parsePagination(req, b)
if err := parsePagination(resp, req, b); err != nil {
return true
}
parseFilter(req, b)
parseReverse(req, b)
return parseWait(resp, req, b)
}

// parsePagination parses the pagination fields for QueryOptions
func parsePagination(req *http.Request, b *structs.QueryOptions) {
func parsePagination(resp http.ResponseWriter, req *http.Request, b *structs.QueryOptions) error {
query := req.URL.Query()
rawPerPage := query.Get("per_page")
if rawPerPage != "" {
perPage, err := strconv.ParseInt(rawPerPage, 10, 32)
if err == nil {
b.PerPage = int32(perPage)
if err != nil {
errMsg := "Expect a number for `per_page` query string parameter"
resp.WriteHeader(http.StatusBadRequest)
resp.Write([]byte(errMsg))
return CodedError(http.StatusBadRequest, errMsg)
}
b.PerPage = int32(perPage)
}

b.NextToken = query.Get("next_token")
return nil
}

// parseFilter parses the filter query parameter for QueryOptions
Expand Down
12 changes: 11 additions & 1 deletion command/agent/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,15 @@ func TestParseConsistency(t *testing.T) {
parseConsistency(resp, req, &b)
must.False(t, b.AllowStale)
must.EqOp(t, 400, resp.Code)
must.EqOp(t, "Expect `true` or `false` for `stale` query string parameter", resp.Body.String())

req, err = http.NewRequest(http.MethodGet, "/v1/jobs?stale=random", nil)
must.NoError(t, err)
resp = httptest.NewRecorder()
parseConsistency(resp, req, &b)
must.False(t, b.AllowStale)
must.EqOp(t, 400, resp.Code)
must.EqOp(t, "Expect `true` or `false` for `stale` query string parameter", resp.Body.String())

b = structs.QueryOptions{}
req, err = http.NewRequest(http.MethodGet, "/v1/catalog/nodes?consistent", nil)
Expand Down Expand Up @@ -721,7 +730,8 @@ func TestParsePagination(t *testing.T) {

require.NoError(t, err)
opts := &structs.QueryOptions{}
parsePagination(req, opts)
resp := httptest.NewRecorder()
parsePagination(resp, req, opts)
require.Equal(t, tc.ExpectedNextToken, opts.NextToken)
require.Equal(t, tc.ExpectedPerPage, opts.PerPage)
})
Expand Down
4 changes: 4 additions & 0 deletions helper/flags/autopilot_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ func (u *UintValue) Set(v string) error {
}

parsed, err := strconv.ParseUint(v, 0, bits.UintSize)
if err != nil {
return err
}

*(u.v) = (uint)(parsed)
return err
}
Expand Down
7 changes: 6 additions & 1 deletion helper/testlog/testlog.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"fmt"
"io"
"log"
"math"
"os"

hclog "github.com/hashicorp/go-hclog"
Expand Down Expand Up @@ -104,7 +105,11 @@ func (w *prefixStderr) Write(p []byte) (int, error) {

// decrease likely hood of partial line writes that may mess up test
// indicator success detection
buf := make([]byte, 0, len(w.prefix)+len(p))
totalLength := len(w.prefix) + len(p)
if totalLength < 0 || totalLength > math.MaxInt32 {
return 0, fmt.Errorf("Total length of the prefix message is out of range: %d", totalLength)
}
buf := make([]byte, 0, totalLength)
buf = append(buf, w.prefix...)
buf = append(buf, p...)

Expand Down
3 changes: 2 additions & 1 deletion helper/users/dynamic/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package dynamic

import (
"fmt"
"math"
"regexp"
"strconv"

Expand Down Expand Up @@ -38,7 +39,7 @@ func Parse(user string) (UGID, error) {
}

i, err := strconv.ParseUint(values[1], 10, 64)
if err != nil {
if err != nil || i > uint64(math.MaxInt32) {
return none, ErrCannotParse
}

Expand Down
Loading