diff mbox series

[v2] cve-check: nvd2 download database variables update

Message ID 20240816122342.974794-1-marta.rybczynska@syslinbit.com
State New
Headers show
Series [v2] cve-check: nvd2 download database variables update | expand

Commit Message

Marta Rybczynska Aug. 16, 2024, 12:23 p.m. UTC
Update the recent code adding local/source database files. Add LOCAL and
SOURCE part to variable names, as none of them needs to be in DL_DIR.
Use old variable names for the source files, so that the change should be
invisible to users after backporting.

At the same time fix a bug: handle a situation when both point to the
same place (was: a deadlock).

Fixes: 03596904392d257572a905a182b92c780d636744 (cve_check: Use a local copy of the database during builds)

Signed-off-by: Marta Rybczynska <marta.rybczynska@syslinbit.com>
---
 meta/classes/cve-check.bbclass                | 14 ++++++-------
 .../meta/cve-update-nvd2-native.bb            | 21 ++++++++++++-------
 2 files changed, 21 insertions(+), 14 deletions(-)

Comments

Richard Purdie Aug. 16, 2024, 1:05 p.m. UTC | #1
On Fri, 2024-08-16 at 14:23 +0200, Marta Rybczynska via lists.openembedded.org wrote:
> Update the recent code adding local/source database files. Add LOCAL and
> SOURCE part to variable names, as none of them needs to be in DL_DIR.
> Use old variable names for the source files, so that the change should be
> invisible to users after backporting.
> 
> At the same time fix a bug: handle a situation when both point to the
> same place (was: a deadlock).
> 
> Fixes: 03596904392d257572a905a182b92c780d636744 (cve_check: Use a local copy of the database during builds)
> 
> Signed-off-by: Marta Rybczynska <marta.rybczynska@syslinbit.com>
> ---
>  meta/classes/cve-check.bbclass                | 14 ++++++-------
>  .../meta/cve-update-nvd2-native.bb            | 21 ++++++++++++-------
>  2 files changed, 21 insertions(+), 14 deletions(-)

I'm not convinced about this I'm afraid. I think do_fetch should put
data into DL_DIR and share it, "cve-update-nvd2" isn't special in that
regard. We do want to have our source artefacts stored in one place
consistently.

If we keep DL_DIR, the naming the makes sense?

Cheers,

Richard
Marta Rybczynska Aug. 16, 2024, 1:18 p.m. UTC | #2
On Fri, Aug 16, 2024 at 3:05 PM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Fri, 2024-08-16 at 14:23 +0200, Marta Rybczynska via
> lists.openembedded.org wrote:
> > Update the recent code adding local/source database files. Add LOCAL and
> > SOURCE part to variable names, as none of them needs to be in DL_DIR.
> > Use old variable names for the source files, so that the change should be
> > invisible to users after backporting.
> >
> > At the same time fix a bug: handle a situation when both point to the
> > same place (was: a deadlock).
> >
> > Fixes: 03596904392d257572a905a182b92c780d636744 (cve_check: Use a local
> copy of the database during builds)
> >
> > Signed-off-by: Marta Rybczynska <marta.rybczynska@syslinbit.com>
> > ---
> >  meta/classes/cve-check.bbclass                | 14 ++++++-------
> >  .../meta/cve-update-nvd2-native.bb            | 21 ++++++++++++-------
> >  2 files changed, 21 insertions(+), 14 deletions(-)
>
> I'm not convinced about this I'm afraid. I think do_fetch should put
> data into DL_DIR and share it, "cve-update-nvd2" isn't special in that
> regard. We do want to have our source artefacts stored in one place
> consistently.
>
> If we keep DL_DIR, the naming the makes sense?
>
>
I'm not sure I understand. I haven't changed the fact that it gets stored in
DL_DIR (after download, because you have moved the download itself to the
local dir
- maybe that wasn't your intent).

More about my use-case, that can give context: I tell many of my customers
to override
the variable, download outside of DL_DIR and then move the database file
manually while
disabling the automatic download.

This is the way I have found to have consistent scan results when running
multiple image
builds in different configurations (various hardware platforms) on machines
that do not
share a filesystem.

Regards,
Marta
Richard Purdie Aug. 16, 2024, 1:30 p.m. UTC | #3
On Fri, 2024-08-16 at 15:18 +0200, Marta Rybczynska wrote:
> 
> 
> On Fri, Aug 16, 2024 at 3:05 PM Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> > On Fri, 2024-08-16 at 14:23 +0200, Marta Rybczynska via lists.openembedded.org wrote:
> > > Update the recent code adding local/source database files. Add LOCAL and
> > > SOURCE part to variable names, as none of them needs to be in DL_DIR.
> > > Use old variable names for the source files, so that the change should be
> > > invisible to users after backporting.
> > > 
> > > At the same time fix a bug: handle a situation when both point to the
> > > same place (was: a deadlock).
> > > 
> > > Fixes: 03596904392d257572a905a182b92c780d636744 (cve_check: Use a local copy of the database during builds)
> > > 
> > > Signed-off-by: Marta Rybczynska <marta.rybczynska@syslinbit.com>
> > > ---
> > >  meta/classes/cve-check.bbclass                | 14 ++++++-------
> > >  .../meta/cve-update-nvd2-native.bb            | 21 ++++++++++++-------
> > >  2 files changed, 21 insertions(+), 14 deletions(-)
> > 
> > I'm not convinced about this I'm afraid. I think do_fetch should put
> > data into DL_DIR and share it, "cve-update-nvd2" isn't special in that
> > regard. We do want to have our source artefacts stored in one place
> > consistently.
> > 
> > If we keep DL_DIR, the naming the makes sense?
> 
> I'm not sure I understand. I haven't changed the fact that it gets stored in
> DL_DIR (after download, because you have moved the download itself to the local dir 
> - maybe that wasn't your intent).

I reached the wrong conclusion from reading the commit message, I
thought you were saying that the file shouldn't be in DL_DIR. You're
saying only that it might not be. We can tweak the commit message to
better explain that, but I'm still not convinced about the naming.

> More about my use-case, that can give context: I tell many of my customers to override
> the variable, download outside of DL_DIR and then move the database file manually while
> disabling the automatic download. 
> 
> This is the way I have found to have consistent scan results when running multiple image
> builds in different configurations (various hardware platforms) on machines that do not
> share a filesystem.

The use case helps thanks. I am worried that "local" doesn't really
make it clear which variable does what. For example, if I want the
build to use a local file, perhaps I need to set the one with "local"
in it?

The trouble with naming is that right now, it is really obvious to us
but in a few months time, this probably won't be clear and new users to
this code will face a similar issue. I therefore do want to get this
naming right.

With my previous patch, I tried to only change what I saw as "internal"
variables which are recipe specific. I'm not a big fan of having to set
global variables to influence a single recipe.

Perhaps people wanting to use a specific database file should just
place it in DL_DIR and configure the code not to update the database
and just use it as is?

Cheers,

Richard
diff mbox series

Patch

diff --git a/meta/classes/cve-check.bbclass b/meta/classes/cve-check.bbclass
index c946de29a4..e66dc43788 100644
--- a/meta/classes/cve-check.bbclass
+++ b/meta/classes/cve-check.bbclass
@@ -32,9 +32,9 @@  CVE_PRODUCT ??= "${BPN}"
 CVE_VERSION ??= "${PV}"
 
 CVE_CHECK_DB_FILENAME ?= "nvdcve_2-1.db"
-CVE_CHECK_DB_DIR ?= "${STAGING_DIR}/CVE_CHECK"
-CVE_CHECK_DB_FILE ?= "${CVE_CHECK_DB_DIR}/${CVE_CHECK_DB_FILENAME}"
-CVE_CHECK_DB_FILE_LOCK ?= "${CVE_CHECK_DB_FILE}.lock"
+CVE_CHECK_DB_LOCAL_DIR ?= "${STAGING_DIR}/CVE_CHECK"
+CVE_CHECK_DB_LOCAL_FILE ?= "${CVE_CHECK_DB_LOCAL_DIR}/${CVE_CHECK_DB_FILENAME}"
+CVE_CHECK_DB_LOCAL_FILE_LOCK ?= "${CVE_CHECK_DB_LOCAL_FILE}.lcl.lock"
 
 CVE_CHECK_LOG ?= "${T}/cve.log"
 CVE_CHECK_TMP_FILE ?= "${TMPDIR}/cve_check"
@@ -183,8 +183,8 @@  python do_cve_check () {
     """
     from oe.cve_check import get_patched_cves
 
-    with bb.utils.fileslocked([d.getVar("CVE_CHECK_DB_FILE_LOCK")], shared=True):
-        if os.path.exists(d.getVar("CVE_CHECK_DB_FILE")):
+    with bb.utils.fileslocked([d.getVar("CVE_CHECK_DB_LOCAL_FILE_LOCK")], shared=True):
+        if os.path.exists(d.getVar("CVE_CHECK_DB_LOCAL_FILE")):
             try:
                 patched_cves = get_patched_cves(d)
             except FileNotFoundError:
@@ -329,7 +329,7 @@  def check_cves(d, patched_cves):
             cve_ignore.append(cve)
 
     import sqlite3
-    db_file = d.expand("file:${CVE_CHECK_DB_FILE}?mode=ro")
+    db_file = d.expand("file:${CVE_CHECK_DB_LOCAL_FILE}?mode=ro")
     conn = sqlite3.connect(db_file, uri=True)
 
     # For each of the known product names (e.g. curl has CPEs using curl and libcurl)...
@@ -438,7 +438,7 @@  def get_cve_info(d, cves):
     import sqlite3
 
     cve_data = {}
-    db_file = d.expand("file:${CVE_CHECK_DB_FILE}?mode=ro")
+    db_file = d.expand("file:${CVE_CHECK_DB_LOCAL_FILE}?mode=ro")
     conn = sqlite3.connect(db_file, uri=True)
 
     for cve in cves:
diff --git a/meta/recipes-core/meta/cve-update-nvd2-native.bb b/meta/recipes-core/meta/cve-update-nvd2-native.bb
index 2d23d28c3e..4ce2933304 100644
--- a/meta/recipes-core/meta/cve-update-nvd2-native.bb
+++ b/meta/recipes-core/meta/cve-update-nvd2-native.bb
@@ -34,9 +34,12 @@  CVE_DB_INCR_UPDATE_AGE_THRES ?= "10368000"
 # Number of attempts for each http query to nvd server before giving up
 CVE_DB_UPDATE_ATTEMPTS ?= "5"
 
-CVE_CHECK_DB_DLDIR_FILE ?= "${DL_DIR}/CVE_CHECK/${CVE_CHECK_DB_FILENAME}"
-CVE_CHECK_DB_DLDIR_LOCK ?= "${CVE_CHECK_DB_DLDIR_FILE}.lock"
-CVE_CHECK_DB_TEMP_FILE ?= "${CVE_CHECK_DB_FILE}.tmp"
+CVE_CHECK_DB_DIR ?= "${DL_DIR}/CVE_CHECK"
+CVE_CHECK_DB_FILE ?= "${CVE_CHECK_DB_DIR}/${CVE_CHECK_DB_FILENAME}"
+CVE_CHECK_DB_SOURCE_FILE ?= "${CVE_CHECK_DB_FILE}"
+CVE_CHECK_DB_SOURCE_LOCK ?= "${CVE_CHECK_DB_SOURCE_FILE}.src.lock"
+# Use a temporary file in the local directory, not next to the source DB
+CVE_CHECK_DB_TEMP_FILE ?= "${CVE_CHECK_DB_LOCAL_FILE}.tmp"
 
 python () {
     if not bb.data.inherits_class("cve-check", d):
@@ -53,7 +56,7 @@  python do_fetch() {
 
     bb.utils.export_proxies(d)
 
-    db_file = d.getVar("CVE_CHECK_DB_DLDIR_FILE")
+    db_file = d.getVar("CVE_CHECK_DB_SOURCE_FILE")
     db_dir = os.path.dirname(db_file)
     db_tmp_file = d.getVar("CVE_CHECK_DB_TEMP_FILE")
 
@@ -91,15 +94,19 @@  python do_fetch() {
         os.remove(db_tmp_file)
 }
 
-do_fetch[lockfiles] += "${CVE_CHECK_DB_DLDIR_LOCK}"
+do_fetch[lockfiles] += "${CVE_CHECK_DB_SOURCE_LOCK}"
 do_fetch[file-checksums] = ""
 do_fetch[vardeps] = ""
 
 python do_unpack() {
     import shutil
-    shutil.copyfile(d.getVar("CVE_CHECK_DB_DLDIR_FILE"), d.getVar("CVE_CHECK_DB_FILE"))
+    try:
+        shutil.copyfile(d.getVar("CVE_CHECK_DB_SOURCE_FILE"), d.getVar("CVE_CHECK_DB_LOCAL_FILE"))
+    except shutil.SameFileError:
+        bb.warn("CVE database source and local file are the same")
+        pass
 }
-do_unpack[lockfiles] += "${CVE_CHECK_DB_DLDIR_LOCK} ${CVE_CHECK_DB_FILE_LOCK}"
+do_unpack[lockfiles] += "${CVE_CHECK_DB_SOURCE_LOCK} ${CVE_CHECK_DB_LOCAL_FILE_LOCK}"
 
 def cleanup_db_download(db_file, db_tmp_file):
     """