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

fix string concat buffer overflow #1806

Merged
merged 3 commits into from
Oct 15, 2022

Conversation

kpango
Copy link
Collaborator

@kpango kpango commented Oct 14, 2022

Signed-off-by: kpango kpango@vdaas.org

Description:

Bug

  • When a user requests multiple searches, each search may fail.
    In the vald process, each stack trace is serialized and recorded in the gRPC status body.
    In this process, vald performs string concatenation by the + operator, but if the string is too long, it may fail.
  • During string concatenation, counting is performed on the string primitive type using the len function, but it is thought that the len function value overflows when the string is cut off.
    This means that we must be careful when joining string types with a total of 2,147,483,647 characters or more.
    Go's strings.Join method does not count the number of characters, but recreates the join by expanding the buffer, which is safer if the string is long enough.

Log

vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway fatal error: string concatenation too long
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway goroutine 95207 [running]:
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway runtime.throw({0x10a632e?, 0x0?})
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	runtime/panic.go:992 +0x71 fp=0xc0011ecda0 sp=0xc0011ecd70 pc=0x4385d1
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway runtime.concatstrings(0xc000179400?, {0xc0011ecf48?, 0x8?, 0x10b1b11?})
vald-lb-gateway-7b7b64cdf4-ftlj6 vald-lb-gateway [32m2022-10-14 08:32:33	[INFO]:	REST server readiness starting on tcp://[::]:3001[39m
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	runtime/string.go:35 +0x256 fp=0xc0011ece18 sp=0xc0011ecda0 pc=0x4538d6
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/info.StackTrace.String(...)
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/info/info.go:336
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc/errdetails.DebugInfoFromInfoDetail(0xc0011ed390)
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/errdetails/errdetails.go:295 +0x489 fp=0xc0011ed1b8 sp=0xc0011ece18 pc=0xa561c9
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc/status.withDetails(0xc000882db0, {0x1376e60, 0xc000882db8}, {0xc0011ed6b8, 0x3, 0x40b00b?})
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/status/status.go:217 +0x808 fp=0xc0011ed470 sp=0xc0011ed1b8 pc=0xa57fe8
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc/status.ParseError({0x1376e60, 0xc000882db8}, 0x5, {0xc000f9e380, 0x37}, {0xc0011ed6b8, 0x3, 0x3})
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/status/status.go:145 +0xa6 fp=0xc0011ed4d8 sp=0xc0011ed470 pc=0xa57246
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/pkg/gateway/lb/handler/grpc.(*server).GetObject.func2.1({0x137e5e0, 0xc0010aed40}, {0xc000a823a0, 0xf}, {0x13865f0, 0xc000dfede0}, {0xc000a8c870, 0x1, 0x1})
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/pkg/gateway/lb/handler/grpc/handler.go:2801 +0x8f8 fp=0xc0011ed8e0 sp=0xc0011ed4d8 pc=0xe4aa18
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/pkg/gateway/lb/service.(*gateway).BroadCast.func2({0x137e5e0, 0xc0010aed40}, {0xc000a823a0, 0xf}, 0xc000ab0380, {0xc000a8c870, 0x1, 0x1})
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/pkg/gateway/lb/service/gateway.go:79 +0x395 fp=0xc0011ed970 sp=0xc0011ed8e0 pc=0xe25035
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc.(*gRPCClient).RangeConcurrent.func2.1.2({0x137e5e0?, 0xc0010aed40?}, 0xc000700000?, {0xc000a8c870?, 0x0?, 0xf1e4e0?})
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/client.go:363 +0x47 fp=0xc0011ed9c0 sp=0xc0011ed970 pc=0xa5ce47
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc.(*gRPCClient).do.func2.1(0xc000b48340?)
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/client.go:558 +0x63 fp=0xc0011eda08 sp=0xc0011ed9c0 pc=0xa5fa43
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc/pool.(*pool).Do(0xc000b48340, 0xc000f98240)
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/pool/pool.go:336 +0x38 fp=0xc0011eda28 sp=0xc0011eda08 pc=0xa4f2f8
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc.(*gRPCClient).do.func2({0x137e5e0?, 0xc0010aed40})
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/client.go:554 +0x146 fp=0xc0011edab0 sp=0xc0011eda28 pc=0xa5f786
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/backoff.(*backoff).Do(0xc000098f60, {0x137e5e0, 0xc0010aed40}, 0xc000f98200)
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/backoff/backoff.go:71 +0x69 fp=0xc0011edce0 sp=0xc0011edab0 pc=0x83c509
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc.(*gRPCClient).do(0xc000aba160, {0x137e5e0, 0xc0010aed40}, {0x1382090, 0xc000b48340}, {0xc000a823a0, 0xf}, 0x1, 0xc001666e20)
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/client.go:553 +0x251 fp=0xc0011eddb0 sp=0xc0011edce0 pc=0xa5f031
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/net/grpc.(*gRPCClient).RangeConcurrent.func2.1()
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/net/grpc/client.go:360 +0x1e7 fp=0xc0011ede98 sp=0xc0011eddb0 pc=0xa5cd47
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/safety.recoverFunc.func1()
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/safety/safety.go:63 +0x73 fp=0xc0011edee0 sp=0xc0011ede98 pc=0x871e13
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway github.com/vdaas/vald/internal/errgroup.(*group).Go.func1()
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/errgroup/group.go:131 +0x22d fp=0xc0011edfe0 sp=0xc0011edee0 pc=0x872c4d
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway runtime.goexit()
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	runtime/asm_amd64.s:1571 +0x1 fp=0xc0011edfe8 sp=0xc0011edfe0 pc=0x46b681
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway created by github.com/vdaas/vald/internal/errgroup.(*group).Go
vald-lb-gateway-7b7b64cdf4-wkxp8 vald-lb-gateway 	github.com/vdaas/vald/internal/errgroup/group.go:115 +0x8e

Solution

  • Replace the string concatenation with the + operator with the strings.Join operation.

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.19.1
  • Docker Version: 20.10.8
  • Kubernetes Version: 1.22.0
  • NGT Version: 1.14.8

Types of changes:

  • Bug fix [type/bug]
  • New feature [type/feature]
  • Add tests [type/test]
  • Security related changes [type/security]
  • Add documents [type/documentation]
  • Refactoring [type/refactoring]
  • Update dependencies [type/dependency]
  • Update benchmarks and performances [type/bench]
  • Update CI [type/ci]

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked open Pull Requests for the similar feature or fixes?
  • I have added tests and benchmarks to cover my changes.
  • I have ensured all new and existing tests passed.
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly.

Signed-off-by: kpango <kpango@vdaas.org>
@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase main
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

hlts2
hlts2 previously approved these changes Oct 14, 2022
Copy link
Contributor

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cloudflare-pages
Copy link

cloudflare-pages bot commented Oct 14, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: fe6625f
Status: ✅  Deploy successful!
Preview URL: https://ef25a1c2.vald.pages.dev
Branch Preview URL: https://bugfix-internal-errdetails-s.vald.pages.dev

View logs

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Base: 30.25% // Head: 30.24% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (fe6625f) compared to base (667b2f4).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1806      +/-   ##
==========================================
- Coverage   30.25%   30.24%   -0.02%     
==========================================
  Files         371      371              
  Lines       34011    34019       +8     
==========================================
- Hits        10291    10288       -3     
- Misses      23306    23315       +9     
- Partials      414      416       +2     
Impacted Files Coverage Δ
internal/client/v1/client/discoverer/discover.go 0.00% <0.00%> (ø)
internal/net/grpc/errdetails/errdetails.go 0.00% <0.00%> (ø)
...ent/core/ngt/service/vqueue/undeleted_index_map.go 68.33% <0.00%> (-2.78%) ⬇️
pkg/agent/core/ngt/service/option.go 89.57% <0.00%> (-1.85%) ⬇️
internal/core/algorithm/ngt/ngt.go 63.93% <0.00%> (+0.64%) ⬆️
internal/worker/queue.go 100.00% <0.00%> (+1.26%) ⬆️
internal/net/http/middleware/timeout.go 93.33% <0.00%> (+2.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: kpango <kpango@vdaas.org>
@github-actions
Copy link
Contributor

@vdaas-ci
Copy link
Collaborator

Profile Report

typevald-agent-ngtvald-lb-gatewayvald-discoverervald-manager-index
cpu
heap
other images

@kpango kpango merged commit 9bcddf0 into main Oct 15, 2022
@kpango kpango deleted the bugfix/internal/errdetails-string-concat-buffer-overflow branch October 15, 2022 02:48
@kpango kpango mentioned this pull request Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants