Am 15.07.20 um 16:34 schrieb Steffan Karger:
> Hi
>
> On 13-07-2020 11:46, Arne Schwabe wrote:
>> @@ -1100,7 +1100,7 @@ process_incoming_link_part1(struct context *c, struct
>> link_socket_info *lsi, boo
>> floated, &ad_start))
>> {
>> /* Restore pre-NCP frame parameters */
>> - if (is_hard_reset(opcode, c->options.key_method))
>> + if (is_hard_reset(opcode, KEY_METHOD_2))
>> {
>
> Can't we just remove the key_method parameter from is_hard_reset()?
Some code still checks if it is a general hard reset or if it is a v2
hard reset. I agree that we can do more cleanup in this area but that
would make more code changes. Should we pull that into this change?
>> @@ -3817,10 +3803,9 @@ options_string(const struct options *o,
>> * tls-auth/tls-crypt does not match. Removing tls-auth here
>> would
>> * break stuff, so leaving that in place. */
>>
>> - if (o->key_method > 1)
>> - {
>> - buf_printf(&out, ",key-method %d", o->key_method);
>> - }
>> +
>> +
>> + buf_printf(&out, ",key-method %d", 2);
>
> This could do with one less newline.
>
>> @@ -2404,7 +2348,7 @@ key_method_2_write(struct buffer *buf, struct
>> tls_session *session)
>> }
>>
>> /* write key_method + flags */
>> - if (!buf_write_u8(buf, (session->opt->key_method & KEY_METHOD_MASK)))
>> + if (!buf_write_u8(buf, (KEY_METHOD_2 & KEY_METHOD_MASK)))
>> {
>> goto error;
>> }
>
> The masking looks a bit silly now. Maybe replace with a static_assert()
> if you want to be sure that no "wrong" bits are set?
Yeah I think the mask is silly here. I will just remove it.
>
>> static bool
>> key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct
>> tls_session *session)
>> {
>> struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
>>
>> - int key_method_flags;
>> bool username_status, password_status;
>>
>> struct gc_arena gc = gc_new();
>> @@ -2582,8 +2464,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi
>> *multi, struct tls_sessio
>> /* allocate temporary objects */
>> ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);
>>
>> - ASSERT(session->opt->key_method == 2);
>> -
>> /* discard leading uint32 */
>> if (!buf_advance(buf, 4))
>> {
>> @@ -2593,7 +2473,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi
>> *multi, struct tls_sessio
>> }
>>
>> /* get key method */
>> - key_method_flags = buf_read_u8(buf);
>> + int key_method_flags = buf_read_u8(buf);
>
> This variable declaration is now *after* a "goto error" jump. Currently
> not harmful, because the variable isn't used after the error label, but
> static analyzers might complain about "jump past initialization".
>
> Otherwise this looks good based on stare-at-code. Didn't have time to
> test yet.
Clang/gcc will already complain/error out if you actually jump past
initialisation. So I think we are good here. Static analyzer are
hopefully not that broken.
Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
