Opened 4 weeks ago

Closed 4 weeks ago

#23643 closed defect (implemented)

Type-check struct members that are passed to confparse

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description


Child Tickets

Change History (12)

comment:1 Changed 4 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 4 weeks ago by nickm

I have a neat hack that does this; Using it, I found a few mistyped values in or_config_t, or_state_t, and sr_disk_state_t. Those appear to be harmless.

comment:3 Changed 4 weeks ago by nickm

Status: acceptedneeds_review

Please review branch typecheck2

comment:4 Changed 4 weeks ago by catalyst

Reviewer: catalyst
Status: needs_reviewneeds_revision

This is a nifty idea.

I'm trying to analyze the standards conformance aspects of it. Doing pointer arithmetic on a null pointer isn't a great idea (and probably non-conforming; offsetof() is allowed to do that because it's part of the implementation). Also pointer subtraction has to be between members of the same array (or a fictitious member one past the end), so subtracting a pointer to a member from a pointer to a member of an included structure doesn't work.

I think it can be salvaged in a standards-conforming way, but I'll have to think about it a bit.

comment:5 Changed 4 weeks ago by catalyst

Suggestions:

  • Instead of using a null pointer, use the address of a static object of the appropriate type. We presumably don't care too much about the extra memory footprint if we're building test code.
  • Use an equality operator instead of pointer subtraction. The equality operators (with both operands being pointers) have the same constraints about type compatibility as subtraction, but don't have the requirement that both pointers point to members of the same array.

comment:6 Changed 4 weeks ago by nickm

Instead of using a null pointer, use the address of a static object of the appropriate type. We presumably don't care too much about the extra memory footprint if we're building test code.

Hm. This part might be harder to do. When I try to do this approach, I get an error: "initializer element is not computable at load time". Apparently comparing the addresses of members of two static objects is not something I can stick into an initializer?

comment:7 Changed 4 weeks ago by catalyst

Oh, a close reading of C99 §6.6 does imply that could be a problem. Address constants should be OK, though, so making var_type_dummy an instance of a union of pointers to all possible types and using named member assignment with the address of the member of the static object might work?

comment:8 Changed 4 weeks ago by nickm

I'm not sure I quite understand what you're suggesting; could you sketch out what it would look like a little?

comment:9 Changed 4 weeks ago by catalyst

Something like this?

union dummyaddrs {
	char **a_str;
	int *a_int;
	double *a_double;
	const char **a_cstr;
};

typedef struct {
	const char *name;
	int typenum;
	union dummyaddrs _;
} config_var_t;

typedef struct {
	char *foostr;
	int fooint;
	const char *foocstr;
} some_opts;

#define optline(name, tnum, checktype) \
	{ #name, tnum, { .checktype = &opts0.name } }

static some_opts opts0;

config_var_t cvars[];

config_var_t cvars[] = {
	optline(fooint, 0, a_int),
	optline(foostr, 0, a_str),
	optline(foocstr, 0, a_cstr),
};

comment:10 Changed 4 weeks ago by nickm

Status: needs_revisionneeds_review

Nice; that does work, and it's simpler too.

I have a new branch as typecheck3.

comment:11 Changed 4 weeks ago by catalyst

Status: needs_reviewmerge_ready

Looks good to me!

Feel free to merge as is, but here are a few nitpicks that will hopefully improve clarity:

  • In the changes file, maybe say "any type mismatches between the C type representing a configuration variable and the C type the data-driven parser uses to store a value there"? The current wording implies that it checks for user errors in a config file.
  • Maybe mention in the comment for CONF_CHECK_VAR_TYPE() that the type checking technique works because a mismatch violates the type compatibility constraint on simple assignment and requires a diagnostic? (That way it's more clear that a conforming compiler will produce some sort of warning and we're not relying on a specific compiler's enthusiastic warning behavior.)
  • Maybe use a macro, possibly named DUMMY_TYPECHECK_INSTANCE, to declare the dummy structs?
  • The int *UINT; /* yes, really. */ comment could be more clear, possibly /* yes, config_assign_value() bounds-checks "UINT" values as 0..INT_MAX */?
  • The non-unit-test variant of CONF_TEST_MEMBERS() should use the same parameter names as the other unit-test variant.
  • Removal of the const qualifier for TransProxyType is not documented in the commit message.

Sorry about the length; this is a tricky technique and I would like us to make it as clear as possible what's going on.

comment:12 Changed 4 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

no problem! I've made the changes you asked for in a typecheck4 branch, and am now merging to master. Thanks again for the reviews!

Note: See TracTickets for help on using tickets.