Package: guix-patches;
Reported by: Nicolas Graves <ngraves <at> ngraves.fr>
Date: Sat, 26 Oct 2024 22:31:02 UTC
Severity: normal
Tags: patch
Done: Nicolas Graves <ngraves <at> ngraves.fr>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Nicolas Graves <ngraves <at> ngraves.fr> Cc: Ludovic Courtès <ludo <at> gnu.org>, 74034 <at> debbugs.gnu.org Subject: [bug#74034] [Nicolas Graves] [PATCH v3 02/17] cve: Separate vendor and string. Date: Wed, 13 Nov 2024 11:53:15 +0900
Hi Nicolas,
Nicolas Graves <ngraves <at> ngraves.fr> writes:
> This commit has currently no proper commit message, but it's because it
> should probably be squashed if we want to go this way.
>
> In the end, I've done it, quite tedious (for me at least!) but done.
> I'm not super sure however that it's clearer (vulnerability-matches?
> definitely is, but the whole, I doubt that). Just pick your preference
> I guess!
I think this one looks nicer without the parsing of colons every time we
need to extract the vendor/package name, thanks for having taken the
time to adjust it based on Ludovic's feedback.
[...]
> (define (configuration-data->cve-configurations alist)
> "Given ALIST, a JSON dictionary for the baroque \"configurations\"
> @@ -232,18 +234,12 @@ (define (vulnerability-matches? vuln vendor hidden-vendors)
> "Checks if a VENDOR matches at least one of <vulnerability> VULN
> packages. When VENDOR is #f, ignore packages that have a vendor among
> HIDDEN-VENDORS."
> - (define (vendor-matches? vendor+name)
> - (if vendor
> - (string-prefix? (string-append vendor ":") vendor+name)
> - (or (null? hidden-vendors)
> - (not (any (cut string-prefix? (string-append <> ":") vendor+name)
> - hidden-vendors)))))
> -
> (match vuln
> (($ <vulnerability> id packages)
> (any (match-lambda
> - (((? vendor-matches? vendor+name) . _) #t)
> - (_ #f))
> + (((? (cut string=? <> vendor)) _) #t)
> + (((? (cut member <> hidden-vendors)) _) #t)
We are comparing <vulnerability> packages to the vendor strings; is this
correct?
At least I'd expect a hidden-vendors match to return #f, since I assume
we do not want to process these further?
> + (_ #f))
> packages))))
>
>
> @@ -290,39 +286,47 @@ (define sexp->vulnerability
> (vulnerability id packages))))
>
> (define (cve-configuration->package-list config)
> - "Parse CONFIG, a config sexp, and return a list of the form (P SEXP)
> -where P is a package name and SEXP expresses constraints on the matching
> -versions."
> + "Parse CONFIG, a config sexp, and return a list of the form (V P SEXP)
> +where V is a CPE vendor, P is a package name and SEXP expresses constraints on
> +the matching versions."
> (let loop ((config config)
> - (packages '()))
> + (vendor+package-list '()))
nitpick: I'm not too found of using the variable type in its name (here,
'list'). Perhaps just 'results' could do (plural to denote it's a
list).
> (match config
> (('or configs ...)
> - (fold loop packages configs))
> - (('and config _ ...) ;XXX
> - (loop config packages))
> - (((? string? package) '_) ;any version
> - (cons `(,package _)
> - (alist-delete package packages)))
> - (((? string? package) sexp)
> - (let ((previous (assoc-ref packages package)))
> - (if previous
> - (cons `(,package (or ,sexp ,@previous))
> - (alist-delete package packages))
> - (cons `(,package ,sexp) packages)))))))
> + (fold loop vendor+package-list configs))
> + (('and config _ ...) ;XXX
> + (loop config vendor+package-list))
> + (((? string? vendor) (? string? package) sexp)
> + (let ((filtered-list (filter (match-lambda
> + ((vendor package _) #f)
I'd use 'remove' to inverse the negative logic. Assuming
'vendor+package-list' becomes 'results', the let-bound variable could
be named (let ((pruned-results (remove ...))) ...)). Also,
shouldn't the '_' in the match-lambda be quoted to denote it's a literal
underscore character, not a "don't-care" pattern?
> + (otherwise otherwise))
> + vendor+package-list)))
> + (match sexp
> + ('_ ;any version
> + (cons `(,vendor ,package _) filtered-list))
> + (_
> + (match (assoc-ref (assoc-ref vendor+package-list vendor) package)
> + ((previous)
> + (cons `(,vendor ,package (or ,sexp ,previous)) filtered-list))
> + (_
> + (cons `(,vendor ,package ,sexp) vendor+package-list))))))))))
I find the logic expressed in this procedure a bit hard to follow, but
that's been inherited from the previous code, so OK.
> (define (merge-package-lists lst)
> - "Merge the list in LST, each of which has the form (p sexp), where P
> -is the name of a package and SEXP is an sexp that constrains matching
> -versions."
> + "Merge the list in LST, each of which has the form (V P SEXP), where V is a
> +CPE vendor, P is the name of a package and SEXP is an sexp that constrains
> +matching versions."
> (fold (lambda (plist result) ;XXX: quadratic
> (fold (match-lambda*
> - (((package version) result)
> - (match (assoc-ref result package)
> - (#f
> - (cons `(,package ,version) result))
> - ((previous)
> - (cons `(,package (or ,version ,previous))
> - (alist-delete package result))))))
> + (((vendor package version) result)
> + (match (assoc-ref result vendor)
> + (((? (cut string=? package <>)) previous)
> + (cons `(,vendor ,package (or ,version ,previous))
> + (filter (match-lambda
> + ((vendor package _) #f)
> + (otherwise otherwise))
> + result)))
This should use SRFI 1's 'remove' instead of 'filter'.
The rest looks good to me.
Could you please address my questions/suggestions and squash this into
the previous commit (with the accompanied changelog commit message
adjustment) ?
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.