Re: [PATCH kmod] libkmod-signature: implement pkcs7 parsing with openssl

[Yauheni Kaliuta <yauheni.kaliuta@redhat.com>]

Hi, Lucas!

>>>>> On Tue, 29 Jan 2019 08:50:10 -0800, Lucas De Marchi  wrote:

 > On Tue, Jan 29, 2019 at 11:50:05AM +0200, Yauheni Kaliuta wrote:
 >> Hi, Lucas!
 >> 
 >>>>>>> On Mon, 28 Jan 2019 10:05:24 -0800, Lucas De Marchi  wrote:
 >> > On Sat, Jan 26, 2019 at 3:01 AM Yauheni Kaliuta
 >> > <yauheni.kaliuta@redhat.com> wrote:
 >> >>
 >> 
 >> [...]
 >> >> >> +
 >> >> >> +       pvt->cms = cms;
 >> >> >> +       pvt->key_id = key_id_str;
 >> >> >> +       pvt->sno = sno_bn;
 >> >> >> +       sig_info->private = pvt;
 >> >>
 >> >> > why do you keep pvt around if the only thing you will do with
 >> >> > it later is to free it?
 >> >> > AFAICS the only thing that needs to remain around is the str
 >> >> > so we can free it after the user used it (because normal
 >> >> > signature is backed in memory by the mem object, while these
 >> >> > are openssl structs)
 >> >>
 >> >> I should keep them until kmod_module_get_info() makes the copies.
 >> >>
 >> >> cms is openssl struct
 >> >> sno_bn is allocated by openssl and must be freed later
 >> >> key_id_str is allocated here since the size in unknown in advance
 >> >> and must be freed later.
 >> >>
 >> >> Or what did I miss?
 >> 
 >> > we could just duplicate the information that we want stored and keep
 >> > the openssl context contained
 >> > to just this function. I thought the only one would be key_str_id, but
 >> > missed that sig and signer
 >> > also need to have their backing object around.
 >> 
 >> If I duplicate it here then without cleanup I'll have memory
 >> leak, no?

 > yes, my idea was to just leave it simpler and add a

 > if (pkcs7)
 >        free(key_id)

Ah, got it, thanks!

Well, if it's not a show stopper for you, I would rather keep it
as in the original proposal since it does not inject
implementation details to the upper level.

 > Lucas De Marchi

 >> 
 >> In the old code they were pointers inside the module image and
 >> freed with the image itself.
 >> 
 >> -- 
 >> WBR,
 >> Yauheni Kaliuta


-- 
WBR,
Yauheni Kaliuta