Hi, On 22/06/18 14:14, Steffan Karger wrote: [cut] >>> +/** >>> + * Encode binary data as PEM >>> + * >>> + * @param name The name to use in the PEM header/footer >>> + * @param dst Destination buffer for PEM-encoded data. Must be a >>> valid >>> + * pointer to an uninitialized buffer structure. Iff this >>> + * function returns true, the buffer will contain memory >>> + * allocated through the supplied gc. >> >> minor: I see the current style is inconsistent wrt having a '.' at the >> end of each doxygen line. Maybe we should decide what to do and stick to >> that ;P But does not need to be changed in this patch of course. > > You sound just like my colleague who's pointing out the missing dots in > my doxygen :') Any preference from your side? I think ending each line > with a dot is best, because that looks best in the generated docs. >
hehe I don't have a strong preference. Let's go for 'always ending with
a dot' then.
[cut]
>>> + if (!PEM_write_bio(bio, name, "", BPTR(src), BLEN(src)))
>>> + {
>>> + ret = false;
>>> + goto cleanup;
>>> + }
>>> +
>>> + BUF_MEM *bptr;
>>> + BIO_get_mem_ptr(bio, &bptr);
>>> +
>>> + *dst = alloc_buf_gc(bptr->length, gc);
>>> + ASSERT(buf_write(dst, bptr->data, bptr->length));
>>
>> why using an ASSERT() here and none in the mbedtls counterpart?
>>
>> If there is no special reason I (personally) think that these helper
>> functions should not use ASSERT() (unless something really bad is
>> happening).
>> Or do you think that a failure here indicates a general memory problem?
>
> Because in the mbedtls part, mbed my change the "outlen" value. While
> here, we create a buffer of bptr->length bytes and then write
> bptr->length bytes to it. If that fails, our entire logic (or
> executable integrity) is broken. It's our code only, and it can not fail.
Yeah, makes sense. Thanks for the explanation. My only worries about
ASSERTs if when the code can be tricked to trigger them without a real
general failure. But this can't be the case.
[cut]
>> Another question: do we have any way to verify that pem_buf contains
>> well-formatted PEM data after pem_encode()?
>> I am asking because if both encode and decode become no-op (because of
>> some bug) we won't be able to realize that.
>>
>> However, I am not sure we have an easy way to check that..maybe you
>> could use some SSL function that would normally load PEM data?
>
> That is a bit annoying to test indeed. How about I just verify that
> this is encoding is not a no-op by adding a check that pem_buf is larger
> that src_buf?
>
Yeah probably enough.
> As for correct PEM en/decoding, I expect that the crypto lib has tested
> that.
I agree.
>
[cut]
>>
>> Cheers,
>
> Thanks!
Thank you!
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
