On Wed, Aug 11, 2021 at 10:52:00AM -0400, Mimi Zohar wrote:

Hi Bruno,

On Tue, 2021-08-10 at 17:28 -0300, Bruno Meneguele wrote:

quoted

The variable "password" is not freed nor returned in case get_password()
succeeds. Instead of using an intermediary variable ("pwd") for returning
the value, use the same "password" var. Issue found by Coverity scan tool.

src/evmctl.c:2565: leaked_storage: Variable "password" going out of scope
    leaks the storage it points to.

Signed-off-by: Bruno Meneguele <redacted>
---
 src/evmctl.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 7a6f2021aa92..b49c7910a4a7 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c

@@ -2601,8 +2601,9 @@ static struct option opts[] = {
 static char *get_password(void)
 {
 	struct termios flags, tmp_flags;
-	char *password, *pwd;
+	char *password;
 	int passlen = 64;
+	bool err = false;
 
 	password = malloc(passlen);
 	if (!password) {

@@ -2622,16 +2623,24 @@ static char *get_password(void)
 	}
 
 	printf("PEM password: ");
-	pwd = fgets(password, passlen, stdin);
+	if (fgets(password, passlen, stdin) == NULL) {
+		perror("fgets");
+		/* we still need to restore the terminal */
+		err = true;
+	}

From the fgets manpage: 
   fgets() returns s on success, and NULL on error
   or  when  end  of  file
   occurs while no characters have been read. 

Yes, I was considering "end of file while no characters have been read"
as an invalid password. The error message is misleading though, which
can be fixed.

quoted

 	/* restore terminal */
 	if (tcsetattr(fileno(stdin), TCSANOW, &flags) != 0) {
 		perror("tcsetattr");
+		err = true;
+	}
+
+	if (err) {
 		free(password);
 		return NULL;
 	}

-	return pwd;
+	return password;

Wouldn't a simpler fix be to test "pwd" here?
        if (!pwd)
                free(password);
        return pwd;

The problem is on success, when 'pwd' is actually not NULL.
With that, I can't free(password). I would need to asprintf(pwd, ...) or
strndup(password). Because of that, I thought it would be cleaner to
remove 'password' completely.

Your call ... :)

thanks,

Mimi

quoted

 }
 
 int main(int argc, char *argv[])

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt