Subject: [bug#74034] [Nicolas Graves] [PATCH v3 02/17] cve: Separate vendor and string.

From: Maxim Cournoyer <[email-masked]>
Date: Wed, 13 Nov 2024 11:53:15 +0900

Hi Nicolas,

Nicolas Graves <[email-masked]> 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