New issue
Advanced search Search tips

Issue 835413 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

vpd_get_value adds newline to values read from its text cache

Project Member Reported by drcrash@chromium.org, Apr 20 2018

Issue description

OS: Chrome OS

What steps will reproduce the problem?
(1) Pick a machine that DOES NOT HAVE /sys/firmware/vpd to repro the bug
(2) Run the following commands in a root shell (values will differ)
   # vpd_get_value serial_number | wc -c
   11
   # VPD_IGNORE_CACHE=1 vpd_get_value serial_number | wc -c
   10

What is the expected result?

The values returned by both commands should be the same.

What happens instead?

The first command produces an extra character (a newline at the end of the value).

 
Components: OS>Systems
The following patch fixes the issue. However it is unclear to me if no code depends on the bad behavior of the extra newline, because on systems without a /sys/firmware/vpd (which is recent) it is very likely that the VPD cache file exists by the time one calls vpd_get_value. Still, this should be fixed (and callers fixed is needed) as this means the command produces values that are inconsistent.

I am happy to put this in a CL.

--- /usr/sbin/vpd_get_value.orig	2018-04-20 11:59:13.565277344 -0700
+++ /usr/sbin/vpd_get_value	2018-04-20 12:11:55.348239634 -0700
@@ -44,7 +44,7 @@
       dump_vpd_log --full --stdout >"${TMP_FILE}"
       CACHE_FILE="${TMP_FILE}"
     fi
-    sed -n "s/^\"${key}\"=\"\(.*\)\"\$/\1/p" "${CACHE_FILE}" | head -n 1
+    printf '%s' "$(sed -n "s/^\"${key}\"=\"\(.*\)\"\$/\1/p" "${CACHE_FILE}" | head -n 1)"
     if [ -e "${TMP_FILE}" ]; then
       rm -f "${TMP_FILE}"

Cc: hungte@chromium.org vapier@chromium.org jorgelo@chromium.org
Labels: -Pri-3 Pri-2
Owner: drcrash@chromium.org
Status: Assigned (was: Untriaged)
CL is https://chromium-review.git.corp.google.com/c/chromiumos/platform/vpd/+/1023111

Comment #1 is incorrect the repro steps will work even if the machine has /sys/firmware/vpd as setting VPD_IGNORE_CACHE=1 will make the script call vpd -g immediately.
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vpd/+/0fe4d9c7c0515a0423844e8369b68183ae0755fb

commit 0fe4d9c7c0515a0423844e8369b68183ae0755fb
Author: Yves Arrouye <drcrash@google.com>
Date: Tue Apr 24 13:22:41 2018

Do not add an extra newline after VPD values taken from cache.

See bug for details. This removes the extra newline that vpd_get_value
adds after a value when it retrieves it from the text cache.

To test, run this:

  # test "$(vpd_get_value serial_number | wc -c)" = \
      "$(VPD_IGNORE_CACHE=1 vpd_get_value serial_number | wc -c)" ||
    echo Still broken
  #

If you see "Still broken" it is... still broken!

BUG= chromium:835413 
TEST=See description above.
BRANCH=none

Change-Id: I0a3560196d96ff5adf27e209cff6cb7e79728971
Reviewed-on: https://chromium-review.googlesource.com/1023111
Commit-Ready: Yves Arrouye <drcrash@chromium.org>
Tested-by: Yves Arrouye <drcrash@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/0fe4d9c7c0515a0423844e8369b68183ae0755fb/util/vpd_get_value

Status: Verified (was: Assigned)

Sign in to add a comment