Skip to content

varlink: ensure that userdata is always valid#18171

Closed
topimiettinen wants to merge 1 commit intosystemd:masterfrom
topimiettinen:varlink-fix-userdata
Closed

varlink: ensure that userdata is always valid#18171
topimiettinen wants to merge 1 commit intosystemd:masterfrom
topimiettinen:varlink-fix-userdata

Conversation

@topimiettinen
Copy link
Copy Markdown
Contributor

a48481d removed zero initialization of struct Varlink in
varlink_new(). However, the callers may (wrongly) use field userdata without
first setting it, so let's set it to NULL.

@topimiettinen topimiettinen added bug 🐛 Programming errors, that need preferential fixing varlink labels Jan 8, 2021
@poettering
Copy link
Copy Markdown
Member

structured initialization in C sets unmentioned fields to zero implicitly, hence you patch changes exactly nothing. We typically only initialize the fields that shall not be zero when using structured initialization.

@topimiettinen
Copy link
Copy Markdown
Contributor Author

The fix could also/instead include one or more of the below

  • add a boolean which is set once userdata is set by the user and display a warning if an attempt is made to get the field before setting, or even assert()
  • fix users by inserting calls to always set the field with varlink_set_userdata(v, NULL)

@topimiettinen
Copy link
Copy Markdown
Contributor Author

Hmm. Could this be a bug in the compiler?

@topimiettinen
Copy link
Copy Markdown
Contributor Author

How about this: varlink_server_add_connection() invoked by connect_callback() sets the Varlink->userdata from VarlinkServer->userdata. In case of resolved, it expects Varlink->userdata to be a DnsQuery but VarlinkServer->userdata is a Manager.

So perhaps the fix could be to change varlink_server_add_connection() to use NULL instead.

@topimiettinen
Copy link
Copy Markdown
Contributor Author

Actually, just remove this line.

In case the callers use struct Varlink field `userdata` without first setting
it, it will contain a non-NULL pointer but possibly pointing to wrong data, so
let's keep the NULL from varlink_new() instead of initializing it from
VarlinkServer->userdata.
@topimiettinen
Copy link
Copy Markdown
Contributor Author

Let's scratch this, it doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐛 Programming errors, that need preferential fixing varlink

Development

Successfully merging this pull request may close these issues.

2 participants