Opened 3 years ago

Closed 3 years ago

#20138 closed task (implemented)

Add constified getters to trunnel

Reported by: asn Owned by: nickm
Priority: Medium Milestone:
Component: Core Tor/Trunnel Version:
Severity: Normal Keywords: trunnel
Cc: Actual Points:
Parent ID: Points: .5
Reviewer: Sponsor:

Description

Here is a feature request for trunnel. Let me know if it does not make sense.

It might be useful to introduce constified versions of getters in trunnel.

As an example, consider the following getter:

/** Return a pointer to the variable-length array field body of 'inp'.
 */
uint8_t * certs_cell_cert_getarray_body(certs_cell_cert_t *inp);

In that spot, we could also additionally add the following getter:

/** Return a const pointer to the variable-length array field body of 'inp'.
 */
const uint8_t * certs_cell_cert_getarray_body_immutable(const certs_cell_cert_t *inp);

which would be more suitable for the following code at channel_tls_process_certs_cell():

...
    uint8_t *cert_body = certs_cell_cert_getarray_body(c);

    if (cert_type > MAX_CERT_TYPE_WANTED)
      continue;

    tor_x509_cert_t *cert = tor_x509_cert_decode(cert_body, cert_len);
...

Basically, everytime you use a getter to get a signature/mac/cert field, you probably want it constified as you dont need to change it when verifying. This occurs quite frequently in the prop224 cells we've been implementing, hence this ticket.

This feature might make some parts of the code more readable/understandable, but at the cost of increasing trunnel's complexity.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by nickm

Points: .5

I'd like the suffix to be "_const" rather than immutable, but otherwise I think this is an okay idea.

Edited to add: wait, no. The infix should be _getconst_ instead of _get_. Otherwise you run into trouble if people create a filed whose name ends with _const or _immutable.

Last edited 3 years ago by nickm (previous) (diff)

comment:2 Changed 3 years ago by nickm

(You would just need to update the AccessorFunctionVisitor class in CodeGen.py to create these prototypes and functions.)

comment:3 Changed 3 years ago by nickm

Status: newneeds_review

Have a look at branch "const_accessor_fns" in my public trunnel repository. Is it what you had in mind? Does it generate good code?

(I need this too.)

comment:4 Changed 3 years ago by asn

Yes, this is precisely what I had in mind indeed!

I tried your branch on my #19043 branch and it worked fine.
I managed to constify three getters that were previously impossible to constify.

I almost feel like the const getter should be the default, and one should call a mutable function to get a non-const pointer. But this is probably too big of a change for now.

comment:5 Changed 3 years ago by nickm

I almost feel like the const getter should be the default, and one should call a mutable function to get a non-const pointer. But this is probably too big of a change for now.

Yeah; I don't really dig breaking backward compatibility. :)

comment:6 Changed 3 years ago by nickm

I've merged this patch to trunnel master, and bumped the trunnel master version to 1.5

comment:7 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.