nss: Skip empty files and avoid use of uninitialized value

[PATCH] nss: Skip empty files and avoid use of uninitialized value

Posted by Jiang XueQian 1 year ago

JSON parser isn't called when reading empty files so `jerr` will be used
uninitialized in the original code. Empty files appear when a network
has no dhcp clients.

This patch checks for such files and skip them.

Signed-off-by: Jiang XueQian <jiangxueqian@gmail.com>
---
 tools/nss/libvirt_nss_leases.c | 5 +++++
 tools/nss/libvirt_nss_macs.c   | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c
index aea81bb56e..7ffe4a8761 100644
--- a/tools/nss/libvirt_nss_leases.c
+++ b/tools/nss/libvirt_nss_leases.c
@@ -290,6 +290,11 @@ findLeases(const char *file,
         jerr = json_tokener_get_error(tok);
     } while (jerr == json_tokener_continue);
 
+    if (nreadTotal == 0) {
+        ret = 0;
+        goto cleanup;
+    }
+
     if (jerr == json_tokener_continue) {
         ERROR("Cannot parse %s: incomplete json found", file);
         goto cleanup;
diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c
index 23229a18f3..57c0f79fbb 100644
--- a/tools/nss/libvirt_nss_macs.c
+++ b/tools/nss/libvirt_nss_macs.c
@@ -152,6 +152,11 @@ findMACs(const char *file,
         jerr = json_tokener_get_error(tok);
     } while (jerr == json_tokener_continue);
 
+    if (nreadTotal == 0) {
+        ret = 0;
+        goto cleanup;
+    }
+
     if (jerr == json_tokener_continue) {
         ERROR("Cannot parse %s: incomplete json found", file);
         goto cleanup;
-- 
2.48.1

Re: [PATCH] nss: Skip empty files and avoid use of uninitialized value

Posted by Michal Prívozník 1 year ago

On 1/18/25 09:32, Jiang XueQian wrote:
> JSON parser isn't called when reading empty files so `jerr` will be used
> uninitialized in the original code. Empty files appear when a network
> has no dhcp clients.
> 
> This patch checks for such files and skip them.
> 
> Signed-off-by: Jiang XueQian <jiangxueqian@gmail.com>
> ---
>  tools/nss/libvirt_nss_leases.c | 5 +++++
>  tools/nss/libvirt_nss_macs.c   | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c
> index aea81bb56e..7ffe4a8761 100644
> --- a/tools/nss/libvirt_nss_leases.c
> +++ b/tools/nss/libvirt_nss_leases.c
> @@ -290,6 +290,11 @@ findLeases(const char *file,
>          jerr = json_tokener_get_error(tok);
>      } while (jerr == json_tokener_continue);
>  
> +    if (nreadTotal == 0) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
>      if (jerr == json_tokener_continue) {
>          ERROR("Cannot parse %s: incomplete json found", file);
>          goto cleanup;
> diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c
> index 23229a18f3..57c0f79fbb 100644
> --- a/tools/nss/libvirt_nss_macs.c
> +++ b/tools/nss/libvirt_nss_macs.c
> @@ -152,6 +152,11 @@ findMACs(const char *file,
>          jerr = json_tokener_get_error(tok);
>      } while (jerr == json_tokener_continue);
>  
> +    if (nreadTotal == 0) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
>      if (jerr == json_tokener_continue) {
>          ERROR("Cannot parse %s: incomplete json found", file);
>          goto cleanup;

We had these checks (well, similar ones) since v5.10.0-rc2^0 but then,
when rewriting libvirt from yajl to json-c these were incorrectly
rewritten to:

  if (nreadTotal > 0 && jerr != json_tokener_success) {
      ERROR("Cannot parse %s: %s", file, json_tokener_error_desc(jerr));
      goto cleanup;
  }

  ret = findMACsFromJSON(jobj, name, macs, nmacs);


I'll be squashing the obvious (trivial) fixup to drop now dead check.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Congratulations on your first libvirt contribution!

Michal

Re: [PATCH] nss: Skip empty files and avoid use of uninitialized value

Posted by Michal Prívozník 1 year ago

On 1/18/25 09:32, Jiang XueQian wrote:
> JSON parser isn't called when reading empty files so `jerr` will be used
> uninitialized in the original code. Empty files appear when a network
> has no dhcp clients.
> 
> This patch checks for such files and skip them.
> 
> Signed-off-by: Jiang XueQian <jiangxueqian@gmail.com>
> ---
>  tools/nss/libvirt_nss_leases.c | 5 +++++
>  tools/nss/libvirt_nss_macs.c   | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c
> index aea81bb56e..7ffe4a8761 100644
> --- a/tools/nss/libvirt_nss_leases.c
> +++ b/tools/nss/libvirt_nss_leases.c
> @@ -290,6 +290,11 @@ findLeases(const char *file,
>          jerr = json_tokener_get_error(tok);
>      } while (jerr == json_tokener_continue);
>  
> +    if (nreadTotal == 0) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
>      if (jerr == json_tokener_continue) {
>          ERROR("Cannot parse %s: incomplete json found", file);
>          goto cleanup;
> diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c
> index 23229a18f3..57c0f79fbb 100644
> --- a/tools/nss/libvirt_nss_macs.c
> +++ b/tools/nss/libvirt_nss_macs.c
> @@ -152,6 +152,11 @@ findMACs(const char *file,
>          jerr = json_tokener_get_error(tok);
>      } while (jerr == json_tokener_continue);
>  
> +    if (nreadTotal == 0) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
>      if (jerr == json_tokener_continue) {
>          ERROR("Cannot parse %s: incomplete json found", file);
>          goto cleanup;

We had these checks (well, similar ones) since v5.10.0-rc2^0 but then,
when rewriting libvirt from yajl to json-c these were incorrectly
rewritten to:

  if (nreadTotal > 0 && jerr != json_tokener_success) {
      ERROR("Cannot parse %s: %s", file, json_tokener_error_desc(jerr));
      goto cleanup;
  }

  ret = findMACsFromJSON(jobj, name, macs, nmacs);


I'll be squashing the obvious (trivial) fixup to drop now dead check.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Congratulations on your first libvirt contribution!

Michal