[libvirt] [PATCH 0/3] Fix NSS issue

Haven't spotted it during review of Dan's patches. Long story short, if there's an unexpected key in JSON parsing stops. But in general a key is hot harmful and shouldn't cause parser to stop. See 1/3 for more explanation. Michal Prívozník (3): nss: Don't stop parsing on unexpected key nss: Include stdio.h and define NULLSTR when debugging is enabled nss: Don't leak memory on parse error tests/nssdata/virbr1.status | 1 + tools/nss/libvirt_nss.h | 2 ++ tools/nss/libvirt_nss_leases.c | 9 +++++---- 3 files changed, 8 insertions(+), 4 deletions(-) -- 2.21.0

Due to latest rewrite of NSS module, we are doing yajl parsing ourselves. This means, we had to introduce couple of callback that yajl calls. According to its documentation, a callback can cancel parsing if it returns a zero value. Well, we do just that in the string callback (findLeasesParserString()). If the JSON file we are parsing contains a key that we are not interested in, zero is returned meaning stop all parsing. This is not correct, because the JSON file can contain some other keys which are not harmful for our address translation (e.g. 'client-id'). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/nssdata/virbr1.status | 1 + tools/nss/libvirt_nss_leases.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/nssdata/virbr1.status b/tests/nssdata/virbr1.status index 4951d4513e..f73c478be0 100644 --- a/tests/nssdata/virbr1.status +++ b/tests/nssdata/virbr1.status @@ -20,6 +20,7 @@ { "ip-address": "192.168.122.3", "mac-address": "52:54:00:aa:bb:cc", + "client-id": "01:52:54:00:aa:bb:cc", "expiry-time": 2000000000 } ] diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 86881641a9..577b5a2fd1 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -201,7 +201,7 @@ findLeasesParserString(void *ctx, if (!(parser->entry.hostname = strndup((char *)stringVal, stringLen))) return 0; } else { - return 0; + return 1; } } else { return 0; -- 2.21.0

On Fri, Aug 09, 2019 at 10:49:09AM +0200, Michal Privoznik wrote:
Due to latest rewrite of NSS module, we are doing yajl parsing ourselves. This means, we had to introduce couple of callback that yajl calls. According to its documentation, a callback can cancel parsing if it returns a zero value. Well, we do just that in the string callback (findLeasesParserString()). If the JSON file we are parsing contains a key that we are not interested in, zero is returned meaning stop all parsing. This is not correct, because the JSON file can contain some other keys which are not harmful for our address translation (e.g. 'client-id').
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/nssdata/virbr1.status | 1 + tools/nss/libvirt_nss_leases.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The NSS module has a compile time option which when enabled makes ERROR() and DEBUG() print messages onto stderr. But now that the module no longer links with libvirt, we need to include stdio.h and define NULLSTR(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h index 63e1bf0af5..ee6c971d3d 100644 --- a/tools/nss/libvirt_nss.h +++ b/tools/nss/libvirt_nss.h @@ -33,6 +33,8 @@ #if 0 # include <errno.h> +# include <stdio.h> +# define NULLSTR(s) ((s) ? (s) : "<null>") # define ERROR(...) \ do { \ char ebuf[1024]; \ -- 2.21.0

On Fri, Aug 09, 2019 at 10:49:10AM +0200, Michal Privoznik wrote:
The NSS module has a compile time option which when enabled makes ERROR() and DEBUG() print messages onto stderr. But now that the module no longer links with libvirt, we need to include stdio.h and define NULLSTR().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss.h | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

If yajl_parse() fails, we try to print an error message. For that, yajl_get_error() is used. However, its documentation say that caller is also responsible for freeing the memory it allocates by using yajl_free_error(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss_leases.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 577b5a2fd1..977e3415f7 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -399,9 +399,10 @@ findLeases(const char *file, if (yajl_parse(parser, (const unsigned char *)line, rv) != yajl_status_ok) { - ERROR("Parse failed %s", - yajl_get_error(parser, 1, - (const unsigned char*)line, rv)); + unsigned char *err = yajl_get_error(parser, 1, + (const unsigned char*)line, rv); + ERROR("Parse failed %s", (const char *) err); + yajl_free_error(parser, err); goto cleanup; } } -- 2.21.0

On Fri, Aug 09, 2019 at 10:49:11AM +0200, Michal Privoznik wrote:
If yajl_parse() fails, we try to print an error message. For that, yajl_get_error() is used. However, its documentation say that caller is also responsible for freeing the memory it allocates by using yajl_free_error().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss_leases.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 577b5a2fd1..977e3415f7 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -399,9 +399,10 @@ findLeases(const char *file,
if (yajl_parse(parser, (const unsigned char *)line, rv) != yajl_status_ok) { - ERROR("Parse failed %s", - yajl_get_error(parser, 1, - (const unsigned char*)line, rv)); + unsigned char *err = yajl_get_error(parser, 1, + (const unsigned char*)line, rv); + ERROR("Parse failed %s", (const char *) err); + yajl_free_error(parser, err); goto cleanup; } }
Same fix needed in libvirt_nss_macs.c too Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 8/9/19 11:04 AM, Daniel P. Berrangé wrote:
On Fri, Aug 09, 2019 at 10:49:11AM +0200, Michal Privoznik wrote:
If yajl_parse() fails, we try to print an error message. For that, yajl_get_error() is used. However, its documentation say that caller is also responsible for freeing the memory it allocates by using yajl_free_error().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss_leases.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 577b5a2fd1..977e3415f7 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -399,9 +399,10 @@ findLeases(const char *file,
if (yajl_parse(parser, (const unsigned char *)line, rv) != yajl_status_ok) { - ERROR("Parse failed %s", - yajl_get_error(parser, 1, - (const unsigned char*)line, rv)); + unsigned char *err = yajl_get_error(parser, 1, + (const unsigned char*)line, rv); + ERROR("Parse failed %s", (const char *) err); + yajl_free_error(parser, err); goto cleanup; } }
Same fix needed in libvirt_nss_macs.c too
Ah, good point. I completely forgot about libvirt_nss_macs.c; The problem I'm fixing in 1/3 applies there too. I'm squashing in the obvious fix (even though macs JSON doesn't have any unexpected keys yet). Thanks, Michal
participants (2)
-
Daniel P. Berrangé
-
Michal Privoznik