[PATCH net-next 0/2] tools: ynl: update NULL pointer handling in generated code

[PATCH net-next 0/2] tools: ynl: update NULL pointer handling in generated code

[Thaison Phan]

This series fixes potential NULL pointer dereferences in YNL-generated C
code during dump list freeing and memory allocation parsing/setters.

Testing:
- Regenerated all c headers/sources under tools/net/ynl/generated/
- Built libynl, ynltool, and test binaries without errors or warnings
- Ran make target run_tests in tools/net/ynl/tests successfully
- Ran tools/testing/selftests/drivers/net/psp.py in QEMU x86_64
  successfully

Thaison Phan (2):
  tools: ynl: check for null ptr on dump free
  tools: ynl: check malloc/calloc fails in generated code

 tools/net/ynl/pyynl/ynl_gen_c.py | 55 ++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 17 deletions(-)

-- 
2.55.0.571.g244d577d93-goog

[PATCH net-next 1/2] tools: ynl: check for null ptr on dump free

[Thaison Phan]

Static analysis detected code paths where freeing a dump list after early
errors when creating the corresponding dump list like in ynl_exec_dump()
can result in a null pointer dereference since the first node in the
ynl_dump_state would still be zero initialized. To prevent this potential
problem updated the ynl c generation script to check for a NULL pointer
before continuing to free the nodes in a dump list.

Signed-off-by: Thaison Phan <thaisonphan@google.com>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index cdc3646f2642..2fd68c738075 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -2747,7 +2747,7 @@ def print_dump_type_free(ri):
     ri.cw.block_start()
     ri.cw.p(f"{sub_type} *next = rsp;")
     ri.cw.nl()
-    ri.cw.block_start(line='while ((void *)next != YNL_LIST_END)')
+    ri.cw.block_start(line='while (next && (void *)next != YNL_LIST_END)')
     _free_type_members_iter(ri, ri.struct['reply'])
     ri.cw.p('rsp = next;')
     ri.cw.p('next = rsp->next;')
-- 
2.55.0.571.g244d577d93-goog

[PATCH net-next 2/2] tools: ynl: check alloc fails in generated code

[Thaison Phan]

Generated YNL code does not check the return value of malloc() and calloc()
before passing the resulting pointer to memcpy(). This could lead to a NULL
pointer dereference on memory allocation failure.

Updated the C code generator to check for allocation failures and to return
an error code when applicable, or to return with no error code for a user
to check for a NULL in the field that allocation was attempted for.

Signed-off-by: Thaison Phan <thaisonphan@google.com>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 53 ++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 2fd68c738075..a323acdc42ba 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -526,16 +526,20 @@ class TypeString(Type):
 
     def _attr_get(self, ri, var):
         len_mem = var + '->_len.' + self.c_name
-        return [f"{len_mem} = len;",
-                f"{var}->{self.c_name} = malloc(len + 1);",
+        return [f"{var}->{self.c_name} = malloc(len + 1);",
+                f"if (!{var}->{self.c_name})",
+                "return YNL_PARSE_CB_ERROR;",
+                f"{len_mem} = len;",
                 f"memcpy({var}->{self.c_name}, ynl_attr_get_str(attr), len);",
                 f"{var}->{self.c_name}[len] = 0;"], \
                ['len = strnlen(ynl_attr_get_str(attr), ynl_attr_data_len(attr));'], \
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
-        return [f"{presence} = strlen({self.c_name});",
-                f"{member} = malloc({presence} + 1);",
+        return [f"{member} = malloc(strlen({self.c_name}) + 1);",
+                f"if (!{member})",
+                "return;",
+                f"{presence} = strlen({self.c_name});",
                 f'memcpy({member}, {self.c_name}, {presence});',
                 f'{member}[{presence}] = 0;']
 
@@ -582,15 +586,19 @@ class TypeBinary(Type):
 
     def _attr_get(self, ri, var):
         len_mem = var + '->_len.' + self.c_name
-        return [f"{len_mem} = len;",
-                f"{var}->{self.c_name} = malloc(len);",
+        return [f"{var}->{self.c_name} = malloc(len);",
+                f"if (!{var}->{self.c_name})",
+                "return YNL_PARSE_CB_ERROR;",
+                f"{len_mem} = len;",
                 f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"], \
                ['len = ynl_attr_data_len(attr);'], \
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
-        return [f"{presence} = len;",
-                f"{member} = malloc({presence});",
+        return [f"{member} = malloc(len);",
+                f"if (!{member})",
+                "return;",
+                f"{presence} = len;",
                 f'memcpy({member}, {self.c_name}, {presence});']
 
 
@@ -601,11 +609,13 @@ class TypeBinaryStruct(TypeBinary):
     def _attr_get(self, ri, var):
         struct_sz = 'sizeof(struct ' + c_lower(self.get("struct")) + ')'
         len_mem = var + '->_' + self.presence_type() + '.' + self.c_name
-        return [f"{len_mem} = len;",
-                f"if (len < {struct_sz})",
+        return [f"if (len < {struct_sz})",
                 f"{var}->{self.c_name} = calloc(1, {struct_sz});",
                 "else",
                 f"{var}->{self.c_name} = malloc(len);",
+                f"if (!{var}->{self.c_name})",
+                "return YNL_PARSE_CB_ERROR;",
+                f"{len_mem} = len;",
                 f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"], \
                ['len = ynl_attr_data_len(attr);'], \
                ['unsigned int len;']
@@ -631,18 +641,21 @@ class TypeBinaryScalarArray(TypeBinary):
 
     def _attr_get(self, ri, var):
         len_mem = var + '->_count.' + self.c_name
-        return [f"{len_mem} = len / sizeof(__{self.get('sub-type')});",
-                f"len = {len_mem} * sizeof(__{self.get('sub-type')});",
+        return [f"len = (len / sizeof(__{self.get('sub-type')})) * sizeof(__{self.get('sub-type')});",
                 f"{var}->{self.c_name} = malloc(len);",
+                f"if (!{var}->{self.c_name})",
+                "return YNL_PARSE_CB_ERROR;",
+                f"{len_mem} = len / sizeof(__{self.get('sub-type')});",
                 f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"], \
                ['len = ynl_attr_data_len(attr);'], \
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
-        return [f"{presence} = count;",
-                f"count *= sizeof(__{self.get('sub-type')});",
-                f"{member} = malloc(count);",
-                f'memcpy({member}, {self.c_name}, count);']
+        return [f"{member} = malloc(count * sizeof(__{self.get('sub-type')}));",
+                f"if (!{member})",
+                "return;",
+                f"{presence} = count;",
+                f'memcpy({member}, {self.c_name}, count * sizeof(__{self.get("sub-type")}));']
 
 
 class TypeBitfield32(Type):
@@ -2227,6 +2240,8 @@ def _multi_parse(ri, struct, init_lines, local_vars):
 
         ri.cw.block_start(line=f"if (n_{aspec.c_name})")
         ri.cw.p(f"dst->{aspec.c_name} = calloc(n_{aspec.c_name}, sizeof(*dst->{aspec.c_name}));")
+        ri.cw.p(f"if (!dst->{aspec.c_name})")
+        ri.cw.p("return YNL_PARSE_CB_ERROR;")
         ri.cw.p(f"dst->_count.{aspec.c_name} = n_{aspec.c_name};")
         ri.cw.p('i = 0;')
         if 'nested-attributes' in aspec:
@@ -2252,6 +2267,8 @@ def _multi_parse(ri, struct, init_lines, local_vars):
         aspec = struct[arg]
         ri.cw.block_start(line=f"if (n_{aspec.c_name})")
         ri.cw.p(f"dst->{aspec.c_name} = calloc(n_{aspec.c_name}, sizeof(*dst->{aspec.c_name}));")
+        ri.cw.p(f"if (!dst->{aspec.c_name})")
+        ri.cw.p("return YNL_PARSE_CB_ERROR;")
         ri.cw.p(f"dst->_count.{aspec.c_name} = n_{aspec.c_name};")
         ri.cw.p('i = 0;')
         if 'nested-attributes' in aspec:
@@ -2275,6 +2292,8 @@ def _multi_parse(ri, struct, init_lines, local_vars):
             ri.cw.nl()
             ri.cw.p('len = strnlen(ynl_attr_get_str(attr), ynl_attr_data_len(attr));')
             ri.cw.p(f'dst->{aspec.c_name}[i] = malloc(sizeof(struct ynl_string) + len + 1);')
+            ri.cw.p(f"if (!dst->{aspec.c_name}[i])")
+            ri.cw.p("return YNL_PARSE_CB_ERROR;")
             ri.cw.p(f"dst->{aspec.c_name}[i]->len = len;")
             ri.cw.p(f"memcpy(dst->{aspec.c_name}[i]->str, ynl_attr_get_str(attr), len);")
             ri.cw.p(f"dst->{aspec.c_name}[i]->str[len] = 0;")
@@ -2434,6 +2453,8 @@ def print_req(ri):
 
     if 'reply' in ri.op[ri.op_mode]:
         ri.cw.p('rsp = calloc(1, sizeof(*rsp));')
+        ri.cw.p('if (!rsp)')
+        ri.cw.p(f'return {ret_err};')
         ri.cw.p('yrs.yarg.data = rsp;')
         ri.cw.p(f"yrs.cb = {op_prefix(ri, 'reply')}_parse;")
         if ri.op.value is not None:
-- 
2.55.0.571.g244d577d93-goog

Re: [PATCH net-next 1/2] tools: ynl: check for null ptr on dump free

[Jakub Kicinski]

On Mon,  3 Aug 2026 20:16:51 +0000 Thaison Phan wrote:
> Static analysis

What's the tool? I guess we can do this but I'd factor it out to 
a separate check at the start of the function. Otherwise it reads
as if there could be a NULL on the list itself.

FWIW NULL is not an empty list, it's a dump error. So the static
tool / LLM is definitely wrong. But I guess free(NULL) is supposed
to be a noop in general so whatever.

Re: [PATCH net-next 2/2] tools: ynl: check alloc fails in generated code

[Jakub Kicinski]

On Mon,  3 Aug 2026 20:16:52 +0000 Thaison Phan wrote:
>      def _setter_lines(self, ri, member, presence):
> -        return [f"{presence} = len;",
> -                f"{member} = malloc({presence});",
> +        return [f"{member} = malloc(len);",
> +                f"if (!{member})",
> +                "return;",
> +                f"{presence} = len;",
>                  f'memcpy({member}, {self.c_name}, {presence});']

Silently eating errors in setters does not seem great, better to crash
and make it clear than things are not working than have a silently
misconfigured system?

AI also points out that setters are preceded by a free (in case we're
replacing existing attr, multiple sets on a single attr are allowed)
So if alloc fails we'll end up with NULL ptr and len from previous
allocation.

Maybe tackle the setters in a separate patch, and keep patch 2 scoped
to the easier case?
-- 
pw-bot: cr

Re: [PATCH net-next 1/2] tools: ynl: check for null ptr on dump free

[Thaison Phan]

Thanks for looking through this.

> What's the tool?

Sashiko (LLM) reported the issue. The path it reported was based on code
output by print_dump() where since the dump state is zero initialized
struct ynl_dump_state yds = {}, yds.first would be NULL. It then pointed
out that in the call to ynl_exec_dump() that there could be a failure in
ynl_msg_end() or send() that would result in `yds.first` staying as NULL,
and the code going to the free_list label where the dump list free would
get passed NULL. I may be misunderstanding the code, but it seemed
legitimate.

> I guess we can do this but I'd factor it out to a separate check at the
> start of the function. Otherwise it reads as if there could be a NULL on
> the list itself.

Factoring out the NULL to be a separate check makes more sense to me and
better shows the intent of the check. I'll update that in v2.

Thanks,
Thaison

Re: [PATCH net-next 2/2] tools: ynl: check alloc fails in generated code

[Thaison Phan]

> Silently eating errors in setters does not seem great, better to crash
> and make it clear than things are not working than have a silently
> misconfigured system?

I was thinking that the user could check for NULL in the struct field that
the setter was allocating for to see if something went wrong, but that is
probably not ideal either since a user might assume with the void return
there can be no errors, and I didn't realize the field is not guaranteed to
be NULL when it is passed in.

> AI also points out that setters are preceded by a free (in case we're
> replacing existing attr, multiple sets on a single attr are allowed) So
> if alloc fails we'll end up with NULL ptr and len from previous
> allocation.

That makes sense, so we could have an unreported free() on the key someone
using the setter might think is still allocated. We could maybe delay the
free() until after the malloc() works, and maybe we could return an error
code in setters that allocate to let users know something went wrong.

> Maybe tackle the setters in a separate patch, and keep patch 2 scoped
> to the easier case?

That sounds like a good idea to me. I'll make v2 only tackle the getters
and look into a different way to handle the setters.

Thanks,
Thaison

Re: [PATCH net-next 1/2] tools: ynl: check for null ptr on dump free

[Jakub Kicinski]

On Wed,  5 Aug 2026 13:14:54 +0000 Thaison Phan wrote:
> Thanks for looking through this.
> 
> > What's the tool?  
> 
> Sashiko (LLM) reported the issue. The path it reported was based on code
> output by print_dump() where since the dump state is zero initialized
> struct ynl_dump_state yds = {}, yds.first would be NULL. It then pointed
> out that in the call to ynl_exec_dump() that there could be a failure in
> ynl_msg_end() or send() that would result in `yds.first` staying as NULL,
> and the code going to the free_list label where the dump list free would
> get passed NULL. I may be misunderstanding the code, but it seemed
> legitimate.

I see it now, you're right. Plumbing the END markers in the list
construction would be a PITA.

> > I guess we can do this but I'd factor it out to a separate check at the
> > start of the function. Otherwise it reads as if there could be a NULL on
> > the list itself.  
> 
> Factoring out the NULL to be a separate check makes more sense to me and
> better shows the intent of the check. I'll update that in v2.
> 
> Thanks,
> Thaison