Re: [PATCH v2] verifiers: fix double close on pgp's sig file descriptor

2018-11-20 Thread Michael Chang
On Fri, Nov 16, 2018 at 03:01:30PM +0100, Daniel Kiper wrote:
> On Thu, Nov 15, 2018 at 06:13:11PM +0800, Michael Chang wrote:
> > An error emerged as when I was tesing the verifiers branch, so instead
> > of putting it in pgp prefix, the verifiers is used to reflect what the
> > patch is based on.
> >
> > While running verify_detached, grub aborts with error.
> >
> > verify_detached /@/.snapshots/1/snapshot/boot/grub/grub.cfg
> > /@/.snapshots/1/snapshot/boot/grub/grub.cfg.sig
> >
> > alloc magic is broken at 0x7beea660: 0
> > Aborted. Press any key to exit.
> >
> > The error is caused by sig file desciptor been closed twice, first time
> > in grub_verify_signature() to which it is passed as parameter. Second in
> > grub_cmd_verify_signature() or in whichever opens the sig file
> > decriptor. The second close is not consider as bug to me either, as in
> > common rule of what opens a file has to close it to avoid file
> > descriptor leakage.
> >
> > Afterall the design of grub_verify_signature() makes it diffcult to keep
> > a good trace on opened file descriptor from it's caller. Let's refine
> > the application interface to accept file path rather than descriptor, in
> > this way the caller doesn't have to care about closing the descriptor by
> > delegating it to grub_verify_signature() with full tracing to opened
> > file descriptor by itself.
> >
> > Signed-off-by: Michael Chang 
> 
> Sadly patch does not apply. Could you rebase it on latest master?

OK. The conflict is caused by new blank line inserted by new commit
which is harmless.

> 
> > v1 -> v2:
> >
> >   - drop change in grub_verify_signature_init()
> 
> If you add a blurb to the commit message why it is needed then I am not
> against it. Even to some extent I am happy with it because it makes code
> a bit nicer.

OK. I'll add that in upcoming v3 patch. :)

Thanks,
Michael

> 
> Daniel
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH v2] verifiers: fix double close on pgp's sig file descriptor

2018-11-16 Thread Daniel Kiper
On Thu, Nov 15, 2018 at 06:13:11PM +0800, Michael Chang wrote:
> An error emerged as when I was tesing the verifiers branch, so instead
> of putting it in pgp prefix, the verifiers is used to reflect what the
> patch is based on.
>
> While running verify_detached, grub aborts with error.
>
> verify_detached /@/.snapshots/1/snapshot/boot/grub/grub.cfg
> /@/.snapshots/1/snapshot/boot/grub/grub.cfg.sig
>
> alloc magic is broken at 0x7beea660: 0
> Aborted. Press any key to exit.
>
> The error is caused by sig file desciptor been closed twice, first time
> in grub_verify_signature() to which it is passed as parameter. Second in
> grub_cmd_verify_signature() or in whichever opens the sig file
> decriptor. The second close is not consider as bug to me either, as in
> common rule of what opens a file has to close it to avoid file
> descriptor leakage.
>
> Afterall the design of grub_verify_signature() makes it diffcult to keep
> a good trace on opened file descriptor from it's caller. Let's refine
> the application interface to accept file path rather than descriptor, in
> this way the caller doesn't have to care about closing the descriptor by
> delegating it to grub_verify_signature() with full tracing to opened
> file descriptor by itself.
>
> Signed-off-by: Michael Chang 

Sadly patch does not apply. Could you rebase it on latest master?

> v1 -> v2:
>
>   - drop change in grub_verify_signature_init()

If you add a blurb to the commit message why it is needed then I am not
against it. Even to some extent I am happy with it because it makes code
a bit nicer.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel