libpod: hostname written to /etc/hostname ends with a newline character #22859

ut-wangbo · 2024-05-31T02:15:44Z

Does this PR introduce a user-facing change?

None

Luap99 · 2024-05-31T11:01:42Z

Please add a test for it, i.e.

diff --git a/test/e2e/run_dns_test.go b/test/e2e/run_dns_test.go
index 341831349..126c17a74 100644
--- a/test/e2e/run_dns_test.go
+++ b/test/e2e/run_dns_test.go
@@ -60,7 +60,7 @@ var _ = Describe("Podman run dns", func() {
                session := podmanTest.Podman([]string{"run", "--hostname=foobar", ALPINE, "cat", "/etc/hostname"})
                session.WaitWithDefaultTimeout()
                Expect(session).Should(ExitCleanly())
-               Expect(session.OutputToString()).To(Equal("foobar"))
+               Expect(string(session.Out.Contents())).To(Equal("foobar\n"))
 
                session = podmanTest.Podman([]string{"run", "--hostname=foobar", ALPINE, "hostname"})
                session.WaitWithDefaultTimeout()

Luap99 · 2024-05-31T11:02:39Z

libpod/container_internal_linux.go

@@ -664,7 +664,7 @@ func (c *Container) makePlatformBindMounts() error {
 	// Make /etc/hostname
 	// This should never change, so no need to recreate if it exists
 	if _, ok := c.state.BindMounts["/etc/hostname"]; !ok {
-		hostnamePath, err := c.writeStringToRundir("hostname", c.Hostname())
+		hostnamePath, err := c.writeStringToRundir("hostname", fmt.Sprintf("%s\n", c.Hostname()))


there isn't really a strong reason for sprintf here, you could do c.Hostname() + "\n" instead

Thanks, looks like the linter wants the spaces removed c.Hostname()+"\n" though

thx, updated

baude · 2024-05-31T13:10:13Z

please add a commit message more than a link

dagraff-cisco

Thanks for letting me know that you had started this PR.

dagraff-cisco · 2024-06-03T11:19:11Z

libpod/container_internal_linux.go

@@ -664,7 +664,7 @@ func (c *Container) makePlatformBindMounts() error {
 	// Make /etc/hostname
 	// This should never change, so no need to recreate if it exists
 	if _, ok := c.state.BindMounts["/etc/hostname"]; !ok {
-		hostnamePath, err := c.writeStringToRundir("hostname", c.Hostname())
+		hostnamePath, err := c.writeStringToRundir("hostname", c.Hostname() + "\n")


Please do not do a string concat like this.

fmt.Sprintf("%s\n", c.Hostname()) instead.

no, it is better like this.
fmt.Sprintf is much slower and this really isn't a complex format where is using Sprintf is required or improves readability

In general, I have seen "bad behaviors" resultant from just string concat. Specific case in point would be SQL injection attacks.

In terms of speed, this is negligible. If I were to be building 1000s of hostnames I would be concerned about the cumulative effect.

Additionally, and I apologize as I'm fairly new to the Go language, I have not gone into great depth of in terms of what the memory consumption would look like with this type of computation.

I'm fine with this either way as it reaches the goal.

In general, I have seen "bad behaviors" resultant from just string concat. Specific case in point would be SQL injection attacks.

Well Sprintf would not guard against that at all.

In terms of speed, this is negligible. If I were to be building 1000s of hostnames I would be concerned about the cumulative effect.

Sure but if you do it everywhere it will get noticeable eventually.
There is simply no benifit of fmt.Sprintf() unless it uses more complex specifiers so we should default to the faster variant.

dagraff-cisco · 2024-06-03T11:19:37Z

test/e2e/run_dns_test.go

@@ -60,7 +60,7 @@ var _ = Describe("Podman run dns", func() {
 		session := podmanTest.Podman([]string{"run", "--hostname=foobar", ALPINE, "cat", "/etc/hostname"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
-		Expect(session.OutputToString()).To(Equal("foobar"))
+		Expect(session.OutputToString()).To(Equal("foobar\n"))


I did not realize that this test existed, otherwise I would have done this in my PR.

Luap99 · 2024-06-03T12:37:49Z

test/e2e/run_dns_test.go

@@ -60,7 +60,7 @@ var _ = Describe("Podman run dns", func() {
 		session := podmanTest.Podman([]string{"run", "--hostname=foobar", ALPINE, "cat", "/etc/hostname"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
-		Expect(session.OutputToString()).To(Equal("foobar"))
+		Expect(session.OutputToString()).To(Equal("foobar\n"))


please use the exact diff I posted this will not work like that because OutputToString() will trim the newline Expect(string(session.Out.Contents())).To(Equal("foobar\n"))

… file debian's man (5) hostname page states "The file should contain a single newline-terminated hostname string." [NO NEW TESTS NEEDED] fix containers#22729 Signed-off-by: Bo Wang <[email protected]>

Luap99

LGTM

openshift-ci · 2024-06-04T10:43:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, ut-wangbo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2024-06-04T17:09:59Z

/lgtm

openshift-ci bot added the release-note-none label May 31, 2024

ut-wangbo force-pushed the hostname-new-line branch from 2b2d189 to 3284240 Compare May 31, 2024 03:46

Luap99 reviewed May 31, 2024

View reviewed changes

ut-wangbo force-pushed the hostname-new-line branch from 3284240 to 63b7330 Compare June 3, 2024 03:40

Luap99 mentioned this pull request Jun 3, 2024

fix: correct creation of container /etc/hostname file #22872

Closed

dagraff-cisco reviewed Jun 3, 2024

View reviewed changes

Luap99 reviewed Jun 3, 2024

View reviewed changes

fix(libpod): add newline character to the end of container's hostname…

7243c71

… file debian's man (5) hostname page states "The file should contain a single newline-terminated hostname string." [NO NEW TESTS NEEDED] fix containers#22729 Signed-off-by: Bo Wang <[email protected]>

ut-wangbo force-pushed the hostname-new-line branch from 63b7330 to 7243c71 Compare June 4, 2024 07:23

Luap99 approved these changes Jun 4, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2024

openshift-ci bot assigned rhatdan Jun 4, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2024

openshift-merge-bot bot merged commit 37339f5 into containers:main Jun 4, 2024
88 of 89 checks passed

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 3, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libpod: hostname written to /etc/hostname ends with a newline character #22859

libpod: hostname written to /etc/hostname ends with a newline character #22859

ut-wangbo commented May 31, 2024

Luap99 commented May 31, 2024

Luap99 May 31, 2024

ut-wangbo Jun 3, 2024

Luap99 Jun 3, 2024

ut-wangbo Jun 4, 2024

baude commented May 31, 2024

dagraff-cisco left a comment

dagraff-cisco Jun 3, 2024

Luap99 Jun 3, 2024

dagraff-cisco Jun 3, 2024

Luap99 Jun 3, 2024

dagraff-cisco Jun 3, 2024

Luap99 Jun 3, 2024

ut-wangbo Jun 4, 2024

Luap99 left a comment

openshift-ci bot commented Jun 4, 2024

rhatdan commented Jun 4, 2024

libpod: hostname written to /etc/hostname ends with a newline character #22859

libpod: hostname written to /etc/hostname ends with a newline character #22859

Conversation

ut-wangbo commented May 31, 2024

Does this PR introduce a user-facing change?

Luap99 commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baude commented May 31, 2024

dagraff-cisco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 4, 2024

rhatdan commented Jun 4, 2024