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

mkcomposefs --from-file ignores last line without newline #327

Closed
cgwalters opened this issue Sep 4, 2024 · 2 comments · Fixed by #331
Closed

mkcomposefs --from-file ignores last line without newline #327

cgwalters opened this issue Sep 4, 2024 · 2 comments · Fixed by #331

Comments

@cgwalters
Copy link
Contributor

It took me an unfortunate amount of time to figure out that there's a bug in tree_from_dump() where basically we ignore the last chunk of a dumpfile if it doesn't end in a newline.

$ cat dump-nonl.txt
/ 4096 40555 222 0 0 10 0.0 - - - trusted.foo1=bar-1 user.foo2=bar-2
/a 0 100644 1 0 0 0 0.0 - - -
/b 0 100644 1 0 0 0 0.0 - - -
$ open /tmp/dump-nonl.txt | base64 -w 0
LyA0MDk2IDQwNTU1IDIyMiAwIDAgMTAgMC4wIC0gLSAtIHRydXN0ZWQuZm9vMT1iYXItMSB1c2VyLmZvbzI9YmFyLTIKL2EgMCAxMDA2NDQgMSAwIDAgMCAwLjAgLSAtIC0KL2IgMCAxMDA2NDQgMSAwIDAgMCAwLjAgLSAtIC0=
$ mkcomposefs --from-file dump-nonl.txt out.cfs
$ composefs-info dump out.cfs
/ 4096 40555 2 0 0 0 0.0 - - - trusted.foo1=bar-1 user.foo2=bar-2
/a 0 100644 1 0 0 0 0.0 - - -
$

The question is...what do we do about it? I guess it's probably unlikely someone was relying on this behavior, but it's hard to say that definitively.

I guess I lean towards changing the behavior to be an error (i.e. a trailing newline is required). But I am curious if others have opinions.

cgwalters added a commit to cgwalters/composefs that referenced this issue Sep 5, 2024
I was doing some more testing and ended up crafting a dumpfile
without a trailing newline. It took me a surprising amount
of time to figure out that we just ignore data without a
trailing newline.

Change things to hard error here for now. If this breaks
someone, we can probably downgrade this to a warning.

Closes: containers#327
Signed-off-by: Colin Walters <walters@verbum.org>
@jeckersb
Copy link
Collaborator

jeckersb commented Sep 5, 2024

I lean towards allowing the no-newline case, only because composefs-dump(5) says:

The file format is very simple, with one file per line[...]

Fields are separated by a single space, and lines by a single newline.

Which as-documented to me only requires them to be separated by a newline but not terminated by one. So I think to keep the previously-published behavior it would be better to allow it. If it's disallowed, then at the very least the docs should be updated to reflect the requirement.

cgwalters added a commit to cgwalters/composefs that referenced this issue Sep 5, 2024
I was doing some more testing and ended up crafting a dumpfile
without a trailing newline. It took me a surprising amount
of time to figure out that we just ignore data without a
trailing newline.

Hopefully, this doesn't break anyone.

Closes: containers#327
Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link
Contributor Author

OK I've updated #331

cgwalters added a commit to cgwalters/composefs that referenced this issue Sep 5, 2024
I was doing some more testing and ended up crafting a dumpfile
without a trailing newline. It took me a surprising amount
of time to figure out that we just ignore data without a
trailing newline.

Hopefully, this doesn't break anyone.

Closes: containers#327
Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters added a commit to cgwalters/composefs that referenced this issue Sep 6, 2024
I was doing some more testing and ended up crafting a dumpfile
without a trailing newline. It took me a surprising amount
of time to figure out that we just ignore data without a
trailing newline.

Hopefully, this doesn't break anyone.

Closes: containers#327
Signed-off-by: Colin Walters <walters@verbum.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants