Skip to content

Commit 03f7b7b

Browse files
iharsuvoraugopherbot
authored andcommitted
x/tools/go/packages: fix Load's "argument list too long" error
The current implementation doesn't assume any limit on the amount of patterns passed to Load which breaks it when called on large codebases. Different platforms have implemented different values for ARG_MAX, https://www.in-ulm.de/~mascheck/various/argmax/#results, https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html, https://devblogs.microsoft.com/oldnewthing/20031210-00/?p=41553. In this fix, we use a conservative estimation of ARG_MAX (32767) that would suit all the supported platforms, subtracted by an approx. length of env on a typical machine (16384). We use this value to split the provided patterns into chunks to run the underlying driver on chunks instead of the whole patterns slice. Fixes golang/go#36909 Change-Id: I7c462c1e905b1f95013afa2b0ce2aa0a435125dc Reviewed-on: https://go-review.googlesource.com/c/tools/+/550477 Reviewed-by: Michael Matloob <[email protected]> Auto-Submit: Michael Matloob <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]>
1 parent 67e856b commit 03f7b7b

File tree

2 files changed

+123
-4
lines changed

2 files changed

+123
-4
lines changed

go/packages/packages.go

Lines changed: 96 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ package packages
99
import (
1010
"context"
1111
"encoding/json"
12+
"errors"
1213
"fmt"
1314
"go/ast"
1415
"go/parser"
@@ -24,6 +25,8 @@ import (
2425
"sync"
2526
"time"
2627

28+
"golang.org/x/sync/errgroup"
29+
2730
"golang.org/x/tools/go/gcexportdata"
2831
"golang.org/x/tools/internal/gocommand"
2932
"golang.org/x/tools/internal/packagesinternal"
@@ -255,8 +258,27 @@ func Load(cfg *Config, patterns ...string) ([]*Package, error) {
255258
// defaultDriver will fall back to the go list driver.
256259
// The boolean result indicates that an external driver handled the request.
257260
func defaultDriver(cfg *Config, patterns ...string) (*DriverResponse, bool, error) {
261+
const (
262+
// windowsArgMax specifies the maximum command line length for
263+
// the Windows' CreateProcess function.
264+
windowsArgMax = 32767
265+
// maxEnvSize is a very rough estimation of the maximum environment
266+
// size of a user.
267+
maxEnvSize = 16384
268+
// safeArgMax specifies the maximum safe command line length to use
269+
// by the underlying driver excl. the environment. We choose the Windows'
270+
// ARG_MAX as the starting point because it's one of the lowest ARG_MAX
271+
// constants out of the different supported platforms,
272+
// e.g., https://www.in-ulm.de/~mascheck/various/argmax/#results.
273+
safeArgMax = windowsArgMax - maxEnvSize
274+
)
275+
chunks, err := splitIntoChunks(patterns, safeArgMax)
276+
if err != nil {
277+
return nil, false, err
278+
}
279+
258280
if driver := findExternalDriver(cfg); driver != nil {
259-
response, err := driver(cfg, patterns...)
281+
response, err := callDriverOnChunks(driver, cfg, chunks)
260282
if err != nil {
261283
return nil, false, err
262284
} else if !response.NotHandled {
@@ -265,11 +287,82 @@ func defaultDriver(cfg *Config, patterns ...string) (*DriverResponse, bool, erro
265287
// (fall through)
266288
}
267289

268-
response, err := goListDriver(cfg, patterns...)
290+
response, err := callDriverOnChunks(goListDriver, cfg, chunks)
269291
if err != nil {
270292
return nil, false, err
271293
}
272-
return response, false, nil
294+
return response, false, err
295+
}
296+
297+
// splitIntoChunks chunks the slice so that the total number of characters
298+
// in a chunk is no longer than argMax.
299+
func splitIntoChunks(patterns []string, argMax int) ([][]string, error) {
300+
if argMax <= 0 {
301+
return nil, errors.New("failed to split patterns into chunks, negative safe argMax value")
302+
}
303+
var chunks [][]string
304+
charsInChunk := 0
305+
nextChunkStart := 0
306+
for i, v := range patterns {
307+
vChars := len(v)
308+
if vChars > argMax {
309+
// a single pattern is longer than the maximum safe ARG_MAX, hardly should happen
310+
return nil, errors.New("failed to split patterns into chunks, a pattern is too long")
311+
}
312+
charsInChunk += vChars + 1 // +1 is for a whitespace between patterns that has to be counted too
313+
if charsInChunk > argMax {
314+
chunks = append(chunks, patterns[nextChunkStart:i])
315+
nextChunkStart = i
316+
charsInChunk = vChars
317+
}
318+
}
319+
// add the last chunk
320+
if nextChunkStart < len(patterns) {
321+
chunks = append(chunks, patterns[nextChunkStart:])
322+
}
323+
return chunks, nil
324+
}
325+
326+
func callDriverOnChunks(driver driver, cfg *Config, chunks [][]string) (*DriverResponse, error) {
327+
if len(chunks) == 0 {
328+
return driver(cfg)
329+
}
330+
responses := make([]*DriverResponse, len(chunks))
331+
errNotHandled := errors.New("driver returned NotHandled")
332+
var g errgroup.Group
333+
for i, chunk := range chunks {
334+
i := i
335+
chunk := chunk
336+
g.Go(func() (err error) {
337+
responses[i], err = driver(cfg, chunk...)
338+
if responses[i] != nil && responses[i].NotHandled {
339+
err = errNotHandled
340+
}
341+
return err
342+
})
343+
}
344+
if err := g.Wait(); err != nil {
345+
if errors.Is(err, errNotHandled) {
346+
return &DriverResponse{NotHandled: true}, nil
347+
}
348+
return nil, err
349+
}
350+
return mergeResponses(responses...), nil
351+
}
352+
353+
func mergeResponses(responses ...*DriverResponse) *DriverResponse {
354+
if len(responses) == 0 {
355+
return nil
356+
}
357+
response := newDeduper()
358+
response.dr.NotHandled = false
359+
response.dr.Compiler = responses[0].Compiler
360+
response.dr.Arch = responses[0].Arch
361+
response.dr.GoVersion = responses[0].GoVersion
362+
for _, v := range responses {
363+
response.addAll(v)
364+
}
365+
return response.dr
273366
}
274367

275368
// A Package describes a loaded Go package.

go/packages/packages_test.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,32 @@ func TestLoadAbsolutePath(t *testing.T) {
367367
}
368368
}
369369

370+
func TestLoadArgumentListIsNotTooLong(t *testing.T) {
371+
// NOTE: this test adds about 2s to the test suite running time
372+
373+
t.Parallel()
374+
375+
// using the real ARG_MAX for some platforms increases the running time of this test by a lot,
376+
// 1_000_000 seems like enough to break Windows and macOS if Load doesn't split provided patterns
377+
argMax := 1_000_000
378+
exported := packagestest.Export(t, packagestest.GOPATH, []packagestest.Module{{
379+
Name: "golang.org/mod",
380+
Files: map[string]interface{}{
381+
"main.go": `package main"`,
382+
}}})
383+
defer exported.Cleanup()
384+
numOfPatterns := argMax/16 + 1 // the pattern below is approx. 16 chars
385+
patterns := make([]string, numOfPatterns)
386+
for i := 0; i < numOfPatterns; i++ {
387+
patterns[i] = fmt.Sprintf("golang.org/mod/p%d", i)
388+
} // patterns have more than argMax number of chars combined with whitespaces b/w patterns
389+
390+
_, err := packages.Load(exported.Config, patterns...)
391+
if err != nil {
392+
t.Fatalf("failed to load: %v", err)
393+
}
394+
}
395+
370396
func TestVendorImports(t *testing.T) {
371397
t.Parallel()
372398

@@ -1308,7 +1334,7 @@ func testNoPatterns(t *testing.T, exporter packagestest.Exporter) {
13081334

13091335
func TestJSON(t *testing.T) { testAllOrModulesParallel(t, testJSON) }
13101336
func testJSON(t *testing.T, exporter packagestest.Exporter) {
1311-
//TODO: add in some errors
1337+
// TODO: add in some errors
13121338
exported := packagestest.Export(t, exporter, []packagestest.Module{{
13131339
Name: "golang.org/fake",
13141340
Files: map[string]interface{}{

0 commit comments

Comments
 (0)