clarify const nature of apis

[Wiladams]

With apis such as the following:
int s2n_config_add_cert_chain_and_key(struct s2n_config *config,
char *cert_chain_pem,
char *private_key_pem);

could you add const to the char * parameters if they are in fact read-only? That will clarify the ownership of the memory, and make it easier to optimize.

[colmmacc]

This is a good ask, but one of the challenges is making sure that the code works with both gcc and clang/llvm. Internally some of the struct members are non-const, because the parameters do change following certain call patterns. What I've found is that it's very difficult to cast away the const-ness (as it should be) in a portable way; sometimes breaking on gcc , sometimes breaking on LLVM. Still, we should figure out an elegant way to do it.

[Wiladams]

I think more fundamentally you need to specify a data contract. If something is supposed to be immutable, that should reflect in the API.

Sent from my Windows Phone

---

From: Colm MacCárthaighmailto:notifications@github.com
Sent: ‎7/‎6/‎2015 8:39 AM
To: awslabs/s2nmailto:s2n@noreply.github.com
Cc: William Adamsmailto:william_a_adams@msn.com
Subject: Re: [s2n] clarify const nature of apis (#128)

This is a good ask, but one of the challenges is making sure that the code works with both gcc and clang/llvm. Internally some of the struct members are non-const, because the parameters do change following certain call patterns. What I've found is that it's very difficult to cast away the const-ness (as it should be) in a portable way; sometimes breaking on gcc , sometimes breaking on LLVM. Still, we should figure out an elegant way to do it.

---

Reply to this email directly or view it on GitHub:
#128 (comment)

[Wiladams]

In the case mentioned above, I would prefer this interface:

int s2n_config_add_cert_chain_and_key(struct s2n_config *config,
const char *cert_chain_pem,
const char *private_key_pem);

This ultimately gets used in this way:

/* Turn the chain into a stuffer */
GUARD(s2n_stuffer_alloc_ro_from_string(&chain_in_stuffer, cert_chain_pem));
GUARD(s2n_stuffer_growable_alloc(&cert_out_stuffer, 2048));

Which ultimately turns into this

int s2n_stuffer_alloc_ro_from_string(struct s2n_stuffer *stuffer, char *str)
{
uint32_t length = strlen(str);
struct s2n_blob b = {.data = (uint8_t *) str,.size = length };

GUARD(s2n_stuffer_alloc(stuffer, length + 1));

return s2n_stuffer_write(stuffer, &b);

}

In this particular case, it would probably be ok to use const all the way down. But there's this fundamental problem because struct s2n_blog is used for both immutable and mutable cases, so you can't make it one way or the other.

Perhaps there should be another object. One is a 'memory fragment', the other is a 'blob'. The difference is that 'memory fragment' is typically a pointer into an already existing chunk of memory that's managed by some other means. This is true in the case of the stuffers, for example, which have a bit for telling whether it was allocated or not. The 'blob', can be assumed to be both mutable, and allocated, so it can be managed directly.

Even this still mixes up 'fragment', and 'read-only', but you can at least get closer. I think you have to have a clear separation of these concerns in the API to prevent easy mistakes of corruption.

[ariccio]

Internally some of the struct members are non-const, because the parameters do change following certain call patterns. What I've found is that it's very difficult to cast away the const-ness (as it should be) in a portable way; sometimes breaking on gcc , sometimes breaking on LLVM.

I'm curious. Elaborate?

[Wiladams]

look at the s2n_blob structure.

struct s2n_blob {
uint8_t *data;
uint32_t size;
};

This core structure is used for the stuffer (a stream interface), which means it MUST be mutable. But, on the other hand, it's also used in various places, like I mention above, where you want to pass a 'const' pointer for the data, because you know it's not going to be changed.

So, do you make it const in the structure, and then cast away const when necessary, or do you keep it the way it is, and cast away const when you're going to use a const pointer?

It's a fundamental data integrity problem.

I would say that if casting away const is involved, you need to rethink the underlying data structures and interfaces.

[ariccio]

Lookin into this a bit further, I can see that some of the internal function calls (of ) aren't const-correct, even though they could be.

For example: s2n_stuffer_alloc_ro_from_string's second parameter, str, could be const, as it's casted (uint8_t*) anyways, and otherwise not written to(?).

EDIT: is that all the uses of private_key_pem (s2n_stuffer_alloc_ro_from_string and strlen)? Am I missing some aliasing going on in/with key_in_stuffer? If I'm not missing something, then private_key_pem can be a const char*? The same for cert_chain_pem?

[Wiladams]

Let's look at that

int s2n_stuffer_alloc_ro_from_string(struct s2n_stuffer *stuffer, char *str)
{
uint32_t length = strlen(str);
struct s2n_blob b = {.data = (uint8_t *) str,.size = length };

GUARD(s2n_stuffer_alloc(stuffer, length + 1));

return s2n_stuffer_write(stuffer, &b);

}

In this case, the s2n_stuffer_write(stuffer, &b) call is the business end of things. It's using that s2n_blob (b) as a temporary because the stuffer_write() call requires it. So, if we just changed that, based on recent changes to the stuffer, it could simply be:

s2n_stuffer_write_bytes(stuffer,str,strlen(str))

In this case,str could become 'const char *str', and within the s2n_stuffer_write_bytes() function, the parameters could become const.

So, in the end, it could be this:

int s2n_stuffer_alloc_ro_from_string(struct s2n_stuffer *stuffer, const char *str)
{
uint32_t length = strlen(str);
GUARD(s2n_stuffer_alloc(stuffer, length + 1));
return s2n_stuffer_write_bytes(stuffer, str, length);
}

See, introducing const in the right place, no 'casting away const', and the data contract is clear, and the temporary variable disappears because it wasn't needed.

[Wiladams]

And the reason this all matters to me is that I'm coming from a scripting perspective. In LuaJIT for example, there's automatic conversion of strings to 'const char *'. So, when the data contract is already 'const char *', or equivalent, it's fairly easy to keep the data contract straight. When the interface is simply 'char *', it's not clear who owns the lifetime of that pointer, and whether the memory pointed to needs to be writable or not. Having it as const at least deals with the writability, and I can use string literals, my lua strings, and all manner of other things.

[ariccio]

+1

+10

[colmmacc]

Our TravisCI build uses llvm and gcc now, so it should be easy enough to tell if a const change breaks with either compiler. The best thing to do here might be to submit a pull-request with any const-ness changes.

[Wiladams]

I've submitted a pull request, to deal with const in the 'write' functions of stuffer primarily. If that gets accepted, I'll tackle more.

[hyandell]

Said pull request was #155 - and was merged. Very much encouraged to tackle more :)

Closing this as the discussion seems to have ended and will be replaced by pull requests; but please reopen if I'm closing this prematurely.