Skip to content

Fixing valid-id in regex#647

Merged
mrunalp merged 1 commit into
opencontainers:masterfrom
rajasec:valid-id
Mar 18, 2016
Merged

Fixing valid-id in regex#647
mrunalp merged 1 commit into
opencontainers:masterfrom
rajasec:valid-id

Conversation

@rajasec

@rajasec rajasec commented Mar 15, 2016

Copy link
Copy Markdown
Contributor

Fixes #635
Also removed _ ,as _ is part of \w
Signed-off-by: rajasec rajasec79@gmail.com

Signed-off-by: rajasec <rajasec79@gmail.com>

var (
idRegex = regexp.MustCompile(`^[\w_-]+$`)
idRegex = regexp.MustCompile(`^[\w-\.]+$`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, I'm fairly sure most regex implementations consider [.] to mean the literal . character (not "any" character, since [.] would be meaningless otherwise).

@rajasec

rajasec commented Mar 15, 2016

Copy link
Copy Markdown
Contributor Author

But without this change, "." is not accepted as valid ID by runc. So I've added part of idregex so that dot is also accepted.

@cyphar

cyphar commented Mar 15, 2016

Copy link
Copy Markdown
Member

Sorry, what I was saying is that you can write it as [\w-.] and it should still work. It's just a nit. :P

@rajasec

rajasec commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

@cyphar
Agree. what you mention will work. Since it gave panic for [\w_-.], so I've added the back tick before the dot operator to be safe.
Let me know whether you are ok with the current change.

@rajasec

rajasec commented Mar 18, 2016

Copy link
Copy Markdown
Contributor Author

@crosbymichael @mrunalp @hqhq
Let me know your comments.

@hqhq

hqhq commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment
@mrunalp

mrunalp commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

LGTM

mrunalp pushed a commit that referenced this pull request Mar 18, 2016
Fixing valid-id in regex
@mrunalp
mrunalp merged commit 54a6e56 into opencontainers:master Mar 18, 2016
@rajasec
rajasec deleted the valid-id branch March 18, 2016 16:50
@ip1981

ip1981 commented Mar 18, 2016

Copy link
Copy Markdown

Honestly, this restriction should not exist. My deployment tools allow some odd ids, my filesystem allows odd file names, and then comes an intermediate layer (runc) telling they cannot be together. Just let it crash, it's out of scope. At least add a comment about why you think this restriction is needed. So nobody will ask again.

@wking

wking commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

On Fri, Mar 18, 2016 at 10:51:59AM -0700, Igor Pashev wrote:

Honestly, this restriction should not exist.

Related discussion in [1,2]. Since this seems like a spec issue
that's more generic than runC, maybe move the discussion into the
mailing list thread 1?

 Subject: [PATCH] runtime: Document container ID charset and uniqueness domain
 Date: Thu, 3 Sep 2015 15:07:33 -0700
 Message-ID: <c08ed73c894899e5ff6c312ecd22de7b4b296e79.1441317770.git.wking@tremily.us>

 Subject: Update cgroupsPath to cgroupName and clarify the
   semantics around that.

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017
…-device-path

config-linux: Add restriction for duplicated device path
@thaJeztah thaJeztah mentioned this pull request Sep 3, 2022
5 tasks
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.

7 participants