Should verify that the max value is not too big #35

rhatdan · 2020-04-27T19:03:18Z

When parsing the ulimit, we should check if the ulimit max value is greater then
the processes max value. If yes then we should return an error.

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2020-04-27T19:03:34Z

@vrothberg FYI

vrothberg · 2020-04-28T05:45:31Z

@AkihiroSuda @vdemeester @thaJeztah PTAL

vrothberg · 2020-04-28T05:47:49Z

ulimit.go

+		var rlimit syscall.Rlimit
+		if err := syscall.Getrlimit(limitType, &rlimit); err == nil {
+			if *hard > int64(rlimit.Max) {
+				return nil, fmt.Errorf("ulimit hard limit must be less than or equal to hard limit for the current process, hard: %d", *hard)


For debugging purposes, it could be helpful to print rlimit.Max as well.

return nil, fmt.Errorf("ulimit hard limit (%d) must be less than or equal to hard limit for the current process (%d)", *hard, rlimit.Max)

vdemeester

LGTM 🐯

thaJeztah · 2020-04-28T09:09:52Z

There's been some discussion around validation in the "go" code versus delegating to the kernel itself; does Linux return an error in this case, or is it silently ignored?

thaJeztah

Actually, I think this will also cause problems in situations where this function is used on a different host than where the process itself lives (e.g. the docker CLI uses this function to parse values, but that would now make the wrong assumption)

For your use-case, would it be possible to perform this validation after parsing the value?

rhatdan · 2020-04-28T17:31:33Z

Sure, the reason I wanted to do this hear was to avoid duplicating the data. Perhaps if we add a different function.

rhatdan · 2020-08-04T10:52:55Z

@thaJeztah Revamped this work, (Going through some old PR's) Now just added a verify function that will check against kernel. The problem is these values are just handed to different OCIs, and they can give you some useless information back to the user.

This should make it easier to diagnose the error, with a better error message.

thaJeztah

thanks for updating! I noticed that the current version would still cause some issues (also still a bit on the fence if the check should be in this code, or only the parsing, but I'm ok with adding)

thaJeztah · 2020-08-04T11:51:22Z

ulimit.go

@@ -108,6 +109,17 @@ func ParseUlimit(val string) (*Ulimit, error) {
 	return &Ulimit{Name: parts[0], Soft: soft, Hard: *hard}, nil
 }

+// Verify that ulimit values work with current kernel
+func (u *Ulimit) Verify() error {
+	var rlimit syscall.Rlimit


This won't compile on Windows (which would break the docker cli). Assuming we only need this on Linux/Unix-y systems (I guess just linux would do for now), could you move this to a _linux.go file (with the right build-tags), and create a stub implementation for other platforms? It could take the same approach as we did for the go-selinux package, and keep the exported function here, and a platform-specific, non-exported function in other files.

thaJeztah · 2020-08-04T11:52:02Z

ulimit_test.go

@@ -21,6 +21,16 @@ func TestParseUlimitValid(t *testing.T) {
 	}
 }

+func TestParseUlimitTooBig(t *testing.T) {


This one should then also be in a ulimit_linux_test.go file

thaJeztah · 2020-08-04T11:54:47Z

ulimit.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+	"syscall"


Should probably be golang.org/x/sys, given that "syscall" is frozen/deprecated in favour of that package

rhatdan · 2020-08-04T20:06:37Z

@thaJeztah Decided to not wait 4 months again to update...

thaJeztah

thanks for updating! code changes look good to me, but unfortunately looks like the test may need some changes 😓 (see my comment inline)

thaJeztah · 2020-08-05T21:43:13Z

ulimit_linux_test.go

+)
+
+func TestParseUlimitTooBig(t *testing.T) {
+	u, err := ParseUlimit("nofile=512:1000024")


I noticed CI isn't running on this repo (have to check why), but testing this locally in a container, the test failed.

--- FAIL: TestParseUlimitTooBig (0.00s) ulimit_linux_test.go:15: expected invalid hard limit

I added a debug line to Verify() (see below), which explains why it failed, because on this machine the limit is higher than what the test expects;

ulimit hard limit (1000024) is less than 1048576

Perhaps this test needs to dynamically set the value to make sure it's higher than the max. That's a bit ugly, so other suggestions welcome 😅)

diff --git a/ulimit_linux.go b/ulimit_linux.go index a56f361..36a4b28 100644 --- a/ulimit_linux.go +++ b/ulimit_linux.go @@ -13,6 +13,7 @@ func (u *Ulimit) Verify() error { if u.Hard > int64(rlimit.Max) { return fmt.Errorf("ulimit hard limit (%d) must be less than or equal to hard limit for the current process %d", u.Hard, rlimit.Max) } + fmt.Printf("ulimit hard limit (%d) is less than %d\n", u.Hard, rlimit.Max) } return nil }

@thaJeztah seems this issue was addressed -- the test case now gets the max value from the kernel.

When parsing the ulimit, we should check if the ulimit max value is greater then the processes max value. If yes then we should return an error. Signed-off-by: Daniel J Walsh <[email protected]>

thaJeztah · 2022-05-17T10:01:11Z

@thaJeztah seems this issue was addressed -- the test case now gets the max value from the kernel.

Ah, forgot about this one.

I'm still a bit on the fence if querying what the kernel supports, and validating against that is within scope of this module and/or if it should be more of a concern of the code using this module (so far the module was used to only parse values, and to validate a correct format, but no other assumptions) 🤔.

What are your thoughts on that, @kolyshkin ?

rhatdan · 2022-05-17T19:51:24Z

Two year old PR, that I totally forgot about. :^(

kolyshkin · 2022-05-18T01:59:26Z

I'm on the verge about it. True, this package is only about parsing, it does not talk to the kernel in any way etc.

OTOH this package knows about ulimit, which is a UNIX concept, and adding Verify method is super easy here, and is not that easy in the other package (since the implementation uses a map which is internal to this package).

Overall I think this should go in, with the only change that Verify should not be specific to Linux -- looks like any unix (including *bsd and darwin) should support it. Perhaps even !windows (need to take a look at whatever x/sys/unix implements).

kolyshkin · 2022-05-18T02:00:03Z

Another thing is, this raises the bar to how this should be tested -- at least we now need to compile this for various GOOS/GOARCH combinations.

thaJeztah · 2022-05-18T12:10:59Z

Yes it's a bit of a tough one; I can definitely see the convenience of having the Validate(). Mostly concerned if it would be widening the scope too much (in addition to the dependency graph).

vrothberg reviewed Apr 28, 2020

View reviewed changes

vdemeester approved these changes Apr 28, 2020

View reviewed changes

thaJeztah requested changes Apr 28, 2020

View reviewed changes

rhatdan force-pushed the master branch from 9bfd2f1 to d36096e Compare April 28, 2020 18:04

rhatdan force-pushed the master branch from 50f05aa to 3b57aa5 Compare August 4, 2020 10:57

AkihiroSuda approved these changes Aug 4, 2020

View reviewed changes

thaJeztah requested changes Aug 4, 2020

View reviewed changes

thaJeztah reviewed Aug 4, 2020

View reviewed changes

rhatdan force-pushed the master branch 4 times, most recently from 579aaaf to 850fcf1 Compare August 4, 2020 20:05

thaJeztah requested changes Aug 5, 2020

View reviewed changes

Add Verify function to check if Ulimit max value is too big

6d777e1

When parsing the ulimit, we should check if the ulimit max value is greater then the processes max value. If yes then we should return an error. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the master branch from 850fcf1 to 6d777e1 Compare August 22, 2020 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Should verify that the max value is not too big #35

Should verify that the max value is not too big #35

rhatdan commented Apr 27, 2020

rhatdan commented Apr 27, 2020

vrothberg commented Apr 28, 2020

vrothberg Apr 28, 2020

vdemeester left a comment

thaJeztah commented Apr 28, 2020

thaJeztah left a comment

rhatdan commented Apr 28, 2020

rhatdan commented Aug 4, 2020

thaJeztah left a comment

thaJeztah Aug 4, 2020

thaJeztah Aug 4, 2020

thaJeztah Aug 4, 2020

rhatdan commented Aug 4, 2020

thaJeztah left a comment

thaJeztah Aug 5, 2020

kolyshkin May 17, 2022

thaJeztah commented May 17, 2022

rhatdan commented May 17, 2022

kolyshkin commented May 18, 2022

kolyshkin commented May 18, 2022

thaJeztah commented May 18, 2022

Should verify that the max value is not too big #35

Are you sure you want to change the base?

Should verify that the max value is not too big #35

Conversation

rhatdan commented Apr 27, 2020

rhatdan commented Apr 27, 2020

vrothberg commented Apr 28, 2020

vrothberg Apr 28, 2020

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 28, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

rhatdan commented Apr 28, 2020

rhatdan commented Aug 4, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Aug 4, 2020

Choose a reason for hiding this comment

thaJeztah Aug 4, 2020

Choose a reason for hiding this comment

thaJeztah Aug 4, 2020

Choose a reason for hiding this comment

rhatdan commented Aug 4, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Aug 5, 2020

Choose a reason for hiding this comment

kolyshkin May 17, 2022

Choose a reason for hiding this comment

thaJeztah commented May 17, 2022

rhatdan commented May 17, 2022

kolyshkin commented May 18, 2022

kolyshkin commented May 18, 2022

thaJeztah commented May 18, 2022