Page MenuHome GnuPG

coverity issues in pinentry
Closed, ResolvedPublic


There are couple of more issues reported by coverity in pinentry that I probably missed in the first run in T5384 last year. Let me know if there is more information needed.



Event Timeline

--- a/pinentry/pinentry.c
+++ b/pinentry/pinentry.c
@@ -351,7 +351,6 @@ get_pid_name_for_uid (unsigned long pid, int uid)
   char *uidstr;
   snprintf (buffer, sizeof buffer, "/proc/%lu/status", pid);
-  buffer[sizeof buffer - 1] = 0;
   fp = fopen (buffer, "rb");
   if (!fp)

Is snprintf guaranteed to write a terminating null byte at the end of buffer if the output is truncated? I guess it does because in gnupg the above pattern isn't used. But in pinentry.c this pattern is used all over the place. So, if the above line is removed, then approx. 10 more similar lines following an snprintf call should be removed as well.

AFAIK the above case has a lot of wiggle room to fit one PID and the surrounded string into 400 bytes and even if it would need to truncate, it would write terminating character, at least on Linux:

The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated. (See also below under NOTES.)

But there might be some historical/compatibility reason to terminate the buffer like this and I can drop that part if needed.

At least old Windows versions did not add a nul in the truncation case. Thus I used to make that sure. I don't think we need it anymore.

I take this ticket. The way to go is removing all such cases.

gniibe triaged this task as Normal priority.Jun 1 2022, 4:35 AM
gniibe added a project: Restricted Project.
gniibe removed a project: Restricted Project.

Fixed in 1.2.1.