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

homectl: make sure we sent the full 8 bytes as flags #31420

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

mrc0mmand
Copy link
Member

Otherwise weird stuff happens on the other side:

[1217111.957263] testsuite-46.sh[61]: + homectl create test-user --disk-size=min --luks-discard=yes --image-path=/home/test-user.home --luks-pbkdf-type=pbkdf2 --luks-pbkdf-time-cost=1ms
[1217112.598219] homectl[66]: Operation on home test-user failed: Provided flags are unsupported (0ad2578000000000).

(taken from TEST-46-HOME run on armv7l)

Fixes issue mentioned in #31419 (comment).

@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Feb 20, 2024
@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed homed homed, homectl, pam_homed and removed please-review PR is ready for (re-)review by a maintainer labels Feb 20, 2024
Otherwise weird stuff happens on the other side:

[1217111.957263] testsuite-46.sh[61]: + homectl create test-user --disk-size=min --luks-discard=yes --image-path=/home/test-user.home --luks-pbkdf-type=pbkdf2 --luks-pbkdf-time-cost=1ms
[1217112.598219] homectl[66]: Operation on home test-user failed: Provided flags are unsupported (0ad2578000000000).

(taken from TEST-46-HOME run on armv7l)

Fixes issue mentioned in systemd#31419 (comment).
@bluca
Copy link
Member

bluca commented Feb 20, 2024

Wait, what? We have tons of flags parameters, does that mean most of those are sending broken messages?

@mrc0mmand
Copy link
Member Author

mrc0mmand commented Feb 20, 2024

Wait, what? We have tons of flags parameters, does that mean most of those are sending broken messages?

If there's a 0 constant together with "t" type, then it's possible, see also c63bfd0.

@bluca
Copy link
Member

bluca commented Feb 20, 2024

Shouldn't the library cast any 't' input automatically then, rather than having every caller do it?

@AdrianVovk
Copy link
Contributor

When we see t we know we must read 8 bytes out of the next argument. The problem is that we've passed in 4, but the function being called has no way of knowing that b/c of the vararg. So ultimately it's reading the 4 bytes we passed in w/ 4 bytes of junk. The function being called has no way to detect this or fix it

@mrc0mmand
Copy link
Member Author

mrc0mmand commented Feb 21, 2024

Yup, as Adrian says, this is an unfortunate (mis?) feature of the va_arg stuff. For example, with this reproducer:

#include <stdarg.h>
#include <stdint.h>
#include <stdio.h>

void vaarg_fun(const char *types, ...) {
        va_list ap;

        va_start(ap, types);
        printf("0x%llx\n", va_arg(ap, uint64_t));
        va_end(ap);
}

int main(void) {
        vaarg_fun("t", 0);
        vaarg_fun("t", UINT64_C(0));
        vaarg_fun("t", 1);
        vaarg_fun("t", UINT64_C(1));
        return 0;
}

I get this output on x86_64:

$ ./test 
0x0
0x0
0x1
0x1

But on armv7l I get this:

$ ./test 
0x4007e4bed77dfc
0x0
0x4007e400000000
0x1

Maybe I should, at least, write a Coccinelle rule, that would catch these cases somewhat automagically.

poettering added a commit to poettering/systemd that referenced this pull request Feb 21, 2024
…essage fields

Since we use varargs for sd_message_append() we need to make sure the
parameters we pass are actually 64bit wide, if "t" is used. Hence cast
appropriately if necessary.

I went through the whole tree, and in most cases we got it right, but
there are some cases we missed so far.

Inspired by: systemd#31420
@poettering
Copy link
Member

also see → #31427

@bluca
Copy link
Member

bluca commented Feb 21, 2024

Maybe I should, at least, write a Coccinelle rule, that would catch these cases somewhat automagically.

That would be very nice, because this is not at all obvious otherwise

@bluca bluca merged commit 8f0dbbd into systemd:main Feb 21, 2024
48 of 49 checks passed
@mrc0mmand mrc0mmand deleted the homectl-armv7l branch February 21, 2024 10:15
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Feb 21, 2024
yuwata pushed a commit that referenced this pull request Feb 21, 2024
…essage fields

Since we use varargs for sd_message_append() we need to make sure the
parameters we pass are actually 64bit wide, if "t" is used. Hence cast
appropriately if necessary.

I went through the whole tree, and in most cases we got it right, but
there are some cases we missed so far.

Inspired by: #31420
ayhamthemayhem pushed a commit to neighbourhoodie/nh-systemd that referenced this pull request Mar 25, 2024
…essage fields

Since we use varargs for sd_message_append() we need to make sure the
parameters we pass are actually 64bit wide, if "t" is used. Hence cast
appropriately if necessary.

I went through the whole tree, and in most cases we got it right, but
there are some cases we missed so far.

Inspired by: systemd#31420
intelfx pushed a commit to intelfx/systemd that referenced this pull request Mar 27, 2024
…essage fields

Since we use varargs for sd_message_append() we need to make sure the
parameters we pass are actually 64bit wide, if "t" is used. Hence cast
appropriately if necessary.

I went through the whole tree, and in most cases we got it right, but
there are some cases we missed so far.

Inspired by: systemd#31420

(cherry picked from commit 04a3af3)
chunyi-wu pushed a commit to chunyi-wu/systemd that referenced this pull request Apr 3, 2024
…essage fields

Since we use varargs for sd_message_append() we need to make sure the
parameters we pass are actually 64bit wide, if "t" is used. Hence cast
appropriately if necessary.

I went through the whole tree, and in most cases we got it right, but
there are some cases we missed so far.

Inspired by: systemd#31420
yuwata pushed a commit to yuwata/systemd that referenced this pull request Apr 26, 2024
…essage fields

Since we use varargs for sd_message_append() we need to make sure the
parameters we pass are actually 64bit wide, if "t" is used. Hence cast
appropriately if necessary.

I went through the whole tree, and in most cases we got it right, but
there are some cases we missed so far.

Inspired by: systemd#31420

(cherry picked from commit 04a3af3)
(cherry picked from commit c0f501c)
yuwata pushed a commit to yuwata/systemd that referenced this pull request Apr 26, 2024
…essage fields

Since we use varargs for sd_message_append() we need to make sure the
parameters we pass are actually 64bit wide, if "t" is used. Hence cast
appropriately if necessary.

I went through the whole tree, and in most cases we got it right, but
there are some cases we missed so far.

Inspired by: systemd#31420

(cherry picked from commit 04a3af3)
(cherry picked from commit c0f501c)
(cherry picked from commit 72b9369)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
homed homed, homectl, pam_homed
Development

Successfully merging this pull request may close these issues.

None yet

5 participants