Re: [PATCH 3/7] virStorageBackendISCSISetAuth: Use g_strndup to '\0' terminate data

On Tue, Jan 31, 2023 at 17:35:59 +0100, Martin Kletzander wrote: > On Tue, Jan 31, 2023 at 05:02:15PM +0100, Peter Krempa wrote: > > Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; > > --- > > src/storage/storage_backend_iscsi.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c > > index e4fa49d05f..01900f6809 100644 > > --- a/src/storage/storage_backend_iscsi.c > > +++ b/src/storage/storage_backend_iscsi.c > > @@ -283,10 +283,8 @@ virStorageBackendISCSISetAuth(const char *portal, > > &secret_value, &secret_size) < 0) > > return -1; > > > > - secret_str = g_new0(char, secret_size + 1); > > - memcpy(secret_str, secret_value, secret_size); > > + secret_str = g_strndup((char *) secret_value, secret_size); > > Unfortunately secrets can contain zero bytes in which case this function > would pad everything after the first zero byte with more zero bytes. > > Fortunately (?) the functions that are called below do not take > secret_size, so it won't affect this particular code block, but we might > have other problems already existing in the code with this. Indeed. If the secret itself contains NUL bytes it would indeed not work properly, but that's pre-existing. But with this patch and a NUL byte in a secret we'd actually write beyond the end of the buffer below when cleaning up as the cleanup is done via virSecureErase(secret_str, secret_size); thus attempting to clear more than the string allocated via g_strndup.

at this point I think I can simply drop this + the other patch doing the same, as the difference is negligible. > > > virSecureErase(secret_value, secret_size); > > - secret_str[secret_size] = '\0'; > > > > if (virISCSINodeUpdate(portal, > > source->devices[0].path, > > -- > > 2.39.1 > >