Message ID | 20240816122342.974794-1-marta.rybczynska@syslinbit.com |
---|---|
State | New |
Headers | show |
Series | [v2] cve-check: nvd2 download database variables update | expand |
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
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
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 --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): """
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(-)