Page MenuHome GnuPG
Create Task
Maniphest T5384

pinentry coverity static analysis reports
Closed, ResolvedPublic
Actions

Description

coverity scan reported couple of issues for pinentry package. The should be addressed with the following patch-set:

pinentry-coverity.patch4 KBDownload

Couple of them are hopefully obvious., but I was not sure with the following one:

Error: USE_AFTER_FREE (CWE-672): [#def9]
pinentry-1.1.1/tty/pinentry-tty.c:555: freed_arg: "fclose" frees "ttyfi".
pinentry-1.1.1/tty/pinentry-tty.c:562: use_closed_file: Calling "fileno" uses file handle "ttyfi" after closing it.
#  560|       }
#  561|   
#  562|->   if (terminal_save (fileno (ttyfi)) < 0)
#  563|       rc = -1;
#  564|   

Error: CPPCHECK_WARNING (CWE-415): [#def10]
pinentry-1.1.1/tty/pinentry-tty.c:588: error[doubleFree]: Resource handle 'ttyfi' freed twice.
#  586|     if (pinentry->ttyname)
#  587|       {
#  588|->       fclose (ttyfi);
#  589|         fclose (ttyfo);
#  590|       }

Revisions and Commits

rP Pinentry
rP7f7fd8bcfd74 tty: Fix error return paths and its resource leaks.
rPa87d9e8f89f9 core,emacs,tty,curses: Fix memory leaks, invalid accese, and mistake.

Related Objects

Event Timeline

Jakuje created this task.Apr 7 2021, 3:00 PM
• gniibe claimed this task.Apr 13 2021, 3:01 AM
• gniibe triaged this task as Normal priority.
• gniibe added a subscriber: • gniibe.

Thank you. I'll take care of this.

• gniibe closed this task as Resolved.Apr 14 2021, 8:58 AM

Applied and pushed.

• gniibe added a commit: rPa87d9e8f89f9: core,emacs,tty,curses: Fix memory leaks, invalid accese, and mistake..Apr 14 2021, 12:37 PM
Jakuje reopened this task as Open.EditedApr 14 2021, 5:44 PM

Thank you for applying the provided changes!

I saw the last issue that I did not provide patch for was not addressed and in corner case, we can still happen to get double-fclose, which should be addressed with the following patch.

pinentry-coverity2.patch950 BDownload

Edit: Updated the patch as the previous version was introducing fd leaks which I missed.

Jakuje added a comment.Apr 15 2021, 11:43 AM

I hope last amendment is the following, which can happen if the tty can be opened only for reading but not for writing:

--- a/tty/pinentry-tty.c
+++ b/tty/pinentry-tty.c
@@ -583,7 +583,8 @@ tty_cmd_handler (pinentry_t pinentry)
   if (pinentry->ttyname)
     {
       fclose (ttyfi);
-      fclose (ttyfo);
+      if (ttyfo)
+        fclose (ttyfo);
     }

   return rc;
• gniibe added a comment.EditedApr 16 2021, 5:02 AM

Actually, calling do_touch_file when some error(s) are not good.
Let me fix all the things.

• gniibe added a commit: rP7f7fd8bcfd74: tty: Fix error return paths and its resource leaks..Apr 16 2021, 5:58 AM
• gniibe added a comment.Apr 16 2021, 5:59 AM

Fixed in rP7f7fd8bcfd74: tty: Fix error return paths and its resource leaks.

• gniibe closed this task as Resolved.Apr 20 2021, 2:28 AM
Jakuje mentioned this in T6007: coverity issues in pinentry.May 30 2022, 3:28 PM