[PATCH 0/3] Fix uninitialized variable in virSocketAddrFormatWithPrefix
This series fixes a bug in virSocketAddrFormatWithPrefix() where an uninitialized variable was being used when masked=false, adds comprehensive test coverage to prevent regressions, and includes a minor code cleanup for consistency. The bug was discovered in the virSocketAddrFormatWithPrefix() function, which is used to format IP addresses with prefix notation (e.g., "1.2.3.4/24"). When the 'masked' parameter was false (meaning the caller wanted to format the original address with a prefix, not the network address), the 'network' variable was left uninitialized, leading to undefined behavior. Julio Faracco (3): util: Fix uninitialized variable in virSocketAddrFormatWithPrefix tests: Add tests for virSocketAddrFormatWithPrefix util: Standardize macro usage to check socket family src/util/virsocketaddr.c | 14 +++++--- tests/sockettest.c | 70 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) -- 2.52.0
The virSocketAddrFormatWithPrefix() function has a bug where the 'network' variable is left uninitialized when masked=false. This occurs because the function only assigns to 'network' inside the masked=true conditional branch. When masked=false, the caller wants to format the original address with a prefix notation (e.g., "1.2.3.4/24") without applying the network mask. However, the code was only initializing 'network' when masking was requested, causing the subsequent virSocketAddrFormat(&network) call to operate on uninitialized data. Fix this by adding an else branch that copies the original address to 'network' when masking is not requested. This ensures 'network' is properly initialized in both code paths. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/util/virsocketaddr.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index f53768878e..80ee3b4c51 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -549,10 +549,14 @@ virSocketAddrFormatWithPrefix(virSocketAddr *addr, return NULL; } - if (masked && virSocketAddrMaskByPrefix(addr, prefix, &network) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failure to mask address")); - return NULL; + if (masked) { + if (virSocketAddrMaskByPrefix(addr, prefix, &network) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failure to mask address")); + return NULL; + } + } else { + network = *addr; } netstr = virSocketAddrFormat(&network); -- 2.52.0
Waiting for feedback, but this is interesting because I don't see any usage of: virSocketAddrFormatWithPrefix(..., ..., false) Perhaps, we need to change the scope of this function... Em seg., 5 de jan. de 2026 às 10:29, Julio Faracco <jcfaracco@gmail.com> escreveu:
The virSocketAddrFormatWithPrefix() function has a bug where the 'network' variable is left uninitialized when masked=false. This occurs because the function only assigns to 'network' inside the masked=true conditional branch.
When masked=false, the caller wants to format the original address with a prefix notation (e.g., "1.2.3.4/24") without applying the network mask. However, the code was only initializing 'network' when masking was requested, causing the subsequent virSocketAddrFormat(&network) call to operate on uninitialized data.
Fix this by adding an else branch that copies the original address to 'network' when masking is not requested. This ensures 'network' is properly initialized in both code paths.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/util/virsocketaddr.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index f53768878e..80ee3b4c51 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -549,10 +549,14 @@ virSocketAddrFormatWithPrefix(virSocketAddr *addr, return NULL; }
- if (masked && virSocketAddrMaskByPrefix(addr, prefix, &network) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failure to mask address")); - return NULL; + if (masked) { + if (virSocketAddrMaskByPrefix(addr, prefix, &network) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failure to mask address")); + return NULL; + } + } else { + network = *addr; }
netstr = virSocketAddrFormat(&network); -- 2.52.0
-- Julio Faracco
On a Monday in 2026, Julio Faracco wrote:
The virSocketAddrFormatWithPrefix() function has a bug where the 'network' variable is left uninitialized when masked=false. This occurs because the function only assigns to 'network' inside the masked=true conditional branch.
When masked=false, the caller wants to format the original address
There is no such caller, ever since its introduction in: commit 426afc0082f1d28449380a5c9260d64ed7183e38 util: rename/move iptablesFormatNetwork to virSocketAddrFormatWithPrefix we always passed masked=true. I think dropping the "masked" argument is easier here. Also, calling it "unitialized" evokes some kind of omission that made the function work by accident. Here, the "addr" is never used so the function would not even work. Jano
with a prefix notation (e.g., "1.2.3.4/24") without applying the network mask. However, the code was only initializing 'network' when masking was requested, causing the subsequent virSocketAddrFormat(&network) call to operate on uninitialized data.
Fix this by adding an else branch that copies the original address to 'network' when masking is not requested. This ensures 'network' is properly initialized in both code paths.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/util/virsocketaddr.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Em sex., 16 de jan. de 2026 às 13:55, Ján Tomko <jtomko@redhat.com> escreveu:
On a Monday in 2026, Julio Faracco wrote:
The virSocketAddrFormatWithPrefix() function has a bug where the 'network' variable is left uninitialized when masked=false. This occurs because the function only assigns to 'network' inside the masked=true conditional branch.
When masked=false, the caller wants to format the original address
There is no such caller, ever since its introduction in: commit 426afc0082f1d28449380a5c9260d64ed7183e38 util: rename/move iptablesFormatNetwork to virSocketAddrFormatWithPrefix we always passed masked=true.
I think dropping the "masked" argument is easier here.
I was thinking of renaming the function name to something like: virSocketAddrFormatWithMask and drop the argument, but I would like to see opinions first. Seems the right way (drop the argument) based on your comments and context.
Also, calling it "unitialized" evokes some kind of omission that made the function work by accident. Here, the "addr" is never used so the function would not even work.
Jano
with a prefix notation (e.g., "1.2.3.4/24") without applying the network mask. However, the code was only initializing 'network' when masking was requested, causing the subsequent virSocketAddrFormat(&network) call to operate on uninitialized data.
Fix this by adding an else branch that copies the original address to 'network' when masking is not requested. This ensures 'network' is properly initialized in both code paths.
ACK. Submitting a V2 with masked code only makes more sense.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/util/virsocketaddr.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Add comprehensive test coverage for virSocketAddrFormatWithPrefix() to verify its behavior with both masked and unmasked address formatting. The tests cover: - IPv4 addresses with various prefix lengths (24, 16, 8) - IPv6 addresses with /64 prefixes - Both masked output (e.g., "1.2.3.4" → "1.2.3.0/24") - And unmasked output (e.g., "1.2.3.4" → "1.2.3.4/24") - Error handling for unsupported AF_UNIX family This test suite validates the fix from the previous commit and prevents regressions in address formatting logic. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- tests/sockettest.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/sockettest.c b/tests/sockettest.c index 5cb8a9fb72..df6ad62b6f 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -55,6 +55,22 @@ static int testFormat(virSocketAddr *addr, const char *addrstr, bool pass) } } +static int testFormatWithPrefix(virSocketAddr *addr, const char *addrstr, + unsigned int prefix, bool masked, bool pass) +{ + g_autofree char *newaddrstr = NULL; + + newaddrstr = virSocketAddrFormatWithPrefix(addr, prefix, masked); + if (!newaddrstr) + return pass ? -1 : 0; + + if (virTestCompareToString(newaddrstr, addrstr) < 0) { + return pass ? -1 : 0; + } else { + return pass ? 0 : -1; + } +} + struct testParseData { virSocketAddr *addr; const char *addrstr; @@ -78,6 +94,20 @@ static int testFormatHelper(const void *opaque) return testFormat(data->addr, data->addrstr, data->pass); } +struct testFormatWithPrefixData { + virSocketAddr *addr; + const char *addrstr; + unsigned int prefix; + bool masked; + bool pass; +}; +static int testFormatWithPrefixHelper(const void *opaque) +{ + const struct testFormatWithPrefixData *data = opaque; + return testFormatWithPrefix(data->addr, data->addrstr, data->prefix, + data->masked, data->pass); +} + static int testRange(const char *saddrstr, const char *eaddrstr, @@ -293,6 +323,32 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX(addrstr, family, prefix, masked, pass) \ + do { \ + virSocketAddr addr = { 0 }; \ + struct testParseData data = { &addr, addrstr, family, pass }; \ + struct testFormatWithPrefixData data2 = { &addr, addrstr, prefix, masked, pass }; \ + if (virTestRun("Test parse " addrstr " family " #family, \ + testParseHelper, &data) < 0) \ + ret = -1; \ + if (virTestRun("Test format " addrstr " family " #family " with prefix /" #prefix, \ + testFormatWithPrefixHelper, &data2) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX(addrstr, addrformated, family, prefix, masked, pass) \ + do { \ + virSocketAddr addr = { 0 }; \ + struct testParseData data = { &addr, addrstr, family, pass }; \ + struct testFormatWithPrefixData data2 = { &addr, addrformated, prefix, masked, pass }; \ + if (virTestRun("Test parse " addrstr " family " #family, \ + testParseHelper, &data) < 0) \ + ret = -1; \ + if (virTestRun("Test format " addrstr " family " #family " with prefix /" #prefix, \ + testFormatWithPrefixHelper, &data2) < 0) \ + ret = -1; \ + } while (0) + #define DO_TEST_RANGE(saddr, eaddr, netaddr, prefix, size, pass) \ do { \ struct testRangeData data \ @@ -376,6 +432,20 @@ mymain(void) DO_TEST_PARSE_AND_FORMAT("::fffe:0:0", AF_UNSPEC, true); DO_TEST_PARSE_AND_FORMAT("::ffff:10.1.2.3", AF_UNSPEC, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("1.2.3.4", "1.2.3.0/24", AF_INET, 24, true, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("1.2.3.4", "1.2.3.4/24", AF_INET, 24, false, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("10.1.1.12", "10.0.0.0/8", AF_INET, 8, true, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("10.1.1.12", "10.1.1.12/8", AF_INET, 8, false, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("192.168.1.124", "192.168.0.0/16", AF_INET, 16, true, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("192.168.1.124", "192.168.1.124/16", AF_INET, 16, false, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("2001:db8:dead:beef:1::", "2001:db8:dead:beef::/64", AF_INET6, 64, true, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("2001:db8:dead:beef:1::", "2001:db8:dead:beef:1::/64", AF_INET6, 64, false, true); + + DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX("1.2.3.4", AF_UNIX, 24, true, false); + DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX("1.2.3.4", AF_UNIX, 24, false, false); + DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX("2001:db8:dead:beef:1::", AF_UNIX, 64, true, false); + DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX("2001:db8:dead:beef:1::", AF_UNIX, 64, false, false); + /* tests that specify a network that should contain the range */ DO_TEST_RANGE("192.168.122.1", "192.168.122.1", "192.168.122.1", 24, 1, true); DO_TEST_RANGE("192.168.122.1", "192.168.122.20", "192.168.122.22", 24, 20, true); -- 2.52.0
Replace direct socket family comparison with the standard VIR_SOCKET_ADDR_IS_FAMILY() macro for consistency. The codebase uses VIR_SOCKET_ADDR_IS_FAMILY() as the standard way to check socket address families. This patch updates one remaining instance that was using direct field access (addr->data.sa.sa_family == AF_UNIX) to use the macro instead. This improves code consistency and makes the code more maintainable by centralizing the family checking logic. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/util/virsocketaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 80ee3b4c51..c5ac1b360a 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -479,7 +479,7 @@ virSocketAddrFormatFull(const virSocketAddr *addr, /* Short-circuit since getnameinfo doesn't work * nicely for UNIX sockets */ - if (addr->data.sa.sa_family == AF_UNIX) { + if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_UNIX)) { if (withService) { addrstr = g_strdup_printf(VIR_LOOPBACK_IPV4_ADDR "%s0", separator ? separator : ":"); -- 2.52.0
On a Monday in 2026, Julio Faracco wrote:
Replace direct socket family comparison with the standard VIR_SOCKET_ADDR_IS_FAMILY() macro for consistency.
The codebase uses VIR_SOCKET_ADDR_IS_FAMILY() as the standard way to check socket address families. This patch updates one remaining
There are many more uses left.
instance that was using direct field access (addr->data.sa.sa_family == AF_UNIX) to use the macro instead.
This improves code consistency and makes the code more maintainable by centralizing the family checking logic.
This paragraph seems unnecessarily verbose. Jano
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/util/virsocketaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 80ee3b4c51..c5ac1b360a 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -479,7 +479,7 @@ virSocketAddrFormatFull(const virSocketAddr *addr,
/* Short-circuit since getnameinfo doesn't work * nicely for UNIX sockets */ - if (addr->data.sa.sa_family == AF_UNIX) { + if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_UNIX)) { if (withService) { addrstr = g_strdup_printf(VIR_LOOPBACK_IPV4_ADDR "%s0", separator ? separator : ":"); -- 2.52.0
Em sex., 16 de jan. de 2026 às 14:02, Ján Tomko <jtomko@redhat.com> escreveu:
On a Monday in 2026, Julio Faracco wrote:
Replace direct socket family comparison with the standard VIR_SOCKET_ADDR_IS_FAMILY() macro for consistency.
The codebase uses VIR_SOCKET_ADDR_IS_FAMILY() as the standard way to check socket address families. This patch updates one remaining
There are many more uses left.
Let me fix the text. Maybe I can submit a single patch since the purpose of this change is just a code clean up in virSocketAddrFormatWithPrefix.
instance that was using direct field access (addr->data.sa.sa_family == AF_UNIX) to use the macro instead.
This improves code consistency and makes the code more maintainable by centralizing the family checking logic.
This paragraph seems unnecessarily verbose.
Jano
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/util/virsocketaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 80ee3b4c51..c5ac1b360a 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -479,7 +479,7 @@ virSocketAddrFormatFull(const virSocketAddr *addr,
/* Short-circuit since getnameinfo doesn't work * nicely for UNIX sockets */ - if (addr->data.sa.sa_family == AF_UNIX) { + if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_UNIX)) { if (withService) { addrstr = g_strdup_printf(VIR_LOOPBACK_IPV4_ADDR "%s0", separator ? separator : ":"); -- 2.52.0
participants (2)
-
Julio Faracco -
Ján Tomko