[5/8] libelf: loop safety: Replace all calls to strcmp

Commit Message

Ian Jackson Dec. 9, 2016, 3:44 p.m. UTC

strcmp can do singificant work, and is found in some inner loops where
we search for the meaning of things we find in the image.  We need to
avoid doing too much work.

So replace all calls to strcmp with elf_strcmp_safe.

Signed-off-by: Ian Jackson <[e-mail]>
---
 xen/common/libelf/libelf-dominfo.c | 37 +++++++++++++++++++------------------
 xen/common/libelf/libelf-private.h |  7 ++++---
 xen/common/libelf/libelf-tools.c   |  4 ++--
 3 files changed, 25 insertions(+), 23 deletions(-)

Comments

Jan Beulich Dec. 12, 2016, 3:22 p.m. UTC | #1

>>> On 09.12.16 at 16:44, <[e-mail]> wrote:
>          /* other */
> -        if ( !strcmp(name, "FEATURES") )
> -            if ( elf_xen_parse_features(value, parms->f_supported,
> +        if ( !elf_strcmp_safe(elf, name, "FEATURES") )
> +            if ( elf_xen_parse_features(elf, value, parms->f_supported,
>                                          parms->f_required) )
>                  return -1;

There must be some patch split problem here - this patch does not
alter the parameters of elf_xen_parse_features(), so the argument
list can't change here.

And one of the then (at least) two patches touching this code could
then take the opportunity and fold the two if()s into one.

Jan

Ian Jackson Dec. 12, 2016, 3:44 p.m. UTC | #2

Jan Beulich writes ("Re: [PATCH 5/8] libelf: loop safety: Replace all calls to strcmp"):
> On 09.12.16 at 16:44, <[e-mail]> wrote:
> >          /* other */
> > -        if ( !strcmp(name, "FEATURES") )
> > -            if ( elf_xen_parse_features(value, parms->f_supported,
> > +        if ( !elf_strcmp_safe(elf, name, "FEATURES") )
> > +            if ( elf_xen_parse_features(elf, value, parms->f_supported,
> >                                          parms->f_required) )
> >                  return -1;
> 
> There must be some patch split problem here - this patch does not
> alter the parameters of elf_xen_parse_features(), so the argument
> list can't change here.

Yes.  That part of this hunk should be in
   libelf: loop safety: Pass `elf' to elf_xen_parse_features
I thought I had done a compile test of that individual patch but
I must have messed that up somehow.  I will move that change.

> And one of the then (at least) two patches touching this code could
> then take the opportunity and fold the two if()s into one.

Looking at the surrounding code, I actually prefer it the way it is.
It makes it more like the other similar test/handle pairs.

Thanks,
Ian.

Patch

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 87a47d9..b037d10 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -70,7 +70,8 @@  elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
             if ( feature[0] == '!' )
             {
                 /* required */
-                if ( !strcmp(feature + 1, elf_xen_feature_names[i]) )
+                if ( !elf_strcmp_safe(elf, feature + 1,
+                                      elf_xen_feature_names[i]) )
                 {
                     elf_xen_feature_set(i, supported);
                     if ( required )
@@ -81,7 +82,7 @@  elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
             else
             {
                 /* supported */
-                if ( !strcmp(feature, elf_xen_feature_names[i]) )
+                if ( !elf_strcmp_safe(elf, feature, elf_xen_feature_names[i]) )
                 {
                     elf_xen_feature_set(i, supported);
                     break;
@@ -173,13 +174,13 @@  elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
         safe_strcpy(parms->xen_ver, str);
         break;
     case XEN_ELFNOTE_PAE_MODE:
-        if ( !strcmp(str, "yes") )
+        if ( !elf_strcmp_safe(elf, str, "yes") )
             parms->pae = XEN_PAE_EXTCR3;
         if ( strstr(str, "bimodal") )
             parms->pae = XEN_PAE_BIMODAL;
         break;
     case XEN_ELFNOTE_BSD_SYMTAB:
-        if ( !strcmp(str, "yes") )
+        if ( !elf_strcmp_safe(elf, str, "yes") )
             parms->bsd_symtab = 1;
         break;
 
@@ -255,7 +256,7 @@  static unsigned elf_xen_parse_notes(struct elf_binary *elf,
         note_name = elf_note_name(elf, note);
         if ( note_name == NULL )
             continue;
-        if ( strcmp(note_name, "Xen") )
+        if ( elf_strcmp_safe(elf, note_name, "Xen") )
             continue;
         if ( elf_xen_parse_note(elf, parms, note) )
             return ELF_NOTE_INVALID;
@@ -315,38 +316,38 @@  elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
         elf_msg(elf, "ELF: %s=\"%s\"\n", name, value);
 
         /* strings */
-        if ( !strcmp(name, "LOADER") )
+        if ( !elf_strcmp_safe(elf, name, "LOADER") )
             safe_strcpy(parms->loader, value);
-        if ( !strcmp(name, "GUEST_OS") )
+        if ( !elf_strcmp_safe(elf, name, "GUEST_OS") )
             safe_strcpy(parms->guest_os, value);
-        if ( !strcmp(name, "GUEST_VER") )
+        if ( !elf_strcmp_safe(elf, name, "GUEST_VER") )
             safe_strcpy(parms->guest_ver, value);
-        if ( !strcmp(name, "XEN_VER") )
+        if ( !elf_strcmp_safe(elf, name, "XEN_VER") )
             safe_strcpy(parms->xen_ver, value);
-        if ( !strcmp(name, "PAE") )
+        if ( !elf_strcmp_safe(elf, name, "PAE") )
         {
-            if ( !strcmp(value, "yes[extended-cr3]") )
+            if ( !elf_strcmp_safe(elf, value, "yes[extended-cr3]") )
                 parms->pae = XEN_PAE_EXTCR3;
             else if ( !strncmp(value, "yes", 3) )
                 parms->pae = XEN_PAE_YES;
         }
-        if ( !strcmp(name, "BSD_SYMTAB") )
+        if ( !elf_strcmp_safe(elf, name, "BSD_SYMTAB") )
             parms->bsd_symtab = 1;
 
         /* longs */
-        if ( !strcmp(name, "VIRT_BASE") )
+        if ( !elf_strcmp_safe(elf, name, "VIRT_BASE") )
             parms->virt_base = strtoull(value, NULL, 0);
-        if ( !strcmp(name, "VIRT_ENTRY") )
+        if ( !elf_strcmp_safe(elf, name, "VIRT_ENTRY") )
             parms->virt_entry = strtoull(value, NULL, 0);
-        if ( !strcmp(name, "ELF_PADDR_OFFSET") )
+        if ( !elf_strcmp_safe(elf, name, "ELF_PADDR_OFFSET") )
             parms->elf_paddr_offset = strtoull(value, NULL, 0);
-        if ( !strcmp(name, "HYPERCALL_PAGE") )
+        if ( !elf_strcmp_safe(elf, name, "HYPERCALL_PAGE") )
             parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) +
                 parms->virt_base;
 
         /* other */
-        if ( !strcmp(name, "FEATURES") )
-            if ( elf_xen_parse_features(value, parms->f_supported,
+        if ( !elf_strcmp_safe(elf, name, "FEATURES") )
+            if ( elf_xen_parse_features(elf, value, parms->f_supported,
                                         parms->f_required) )
                 return -1;
     }
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 388c3da..082c572 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -98,9 +98,10 @@  do { strncpy((d),(s),sizeof((d))-1);            \
 #define memset  MISTAKE_unspecified_memset
 #define memmove MISTAKE_unspecified_memmove
 #define strcpy  MISTAKE_unspecified_strcpy
-  /* This prevents libelf from using these undecorated versions
-   * of memcpy, memset, memmove and strcpy.  Every call site
-   * must either use elf_mem*_unchecked, or elf_mem*_safe. */
+#define strcmp  MISTAKE_unspecified_strcmp
+ /* This prevents libelf from using these undecorated versions
+   * of memcpy, memset, memmove, strcpy and strcmp.  Every call site
+   * must either use elf_mem*_unchecked, or elf_*_safe. */
 
 #endif /* __LIBELF_PRIVATE_H__ */
 
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index ab83150..7fa5963 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -162,7 +162,7 @@  ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
             /* input has an insane section header count field */
             break;
         sname = elf_section_name(elf, shdr);
-        if ( sname && !strcmp(sname, name) )
+        if ( sname && !elf_strcmp_safe(elf, sname, name) )
             return shdr;
     }
     return ELF_INVALID_HANDLE(elf_shdr);
@@ -274,7 +274,7 @@  ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
         sym_name = elf_strval(elf, elf->sym_strtab + name);
         if ( sym_name == NULL ) /* out of range, oops */
             return ELF_INVALID_HANDLE(elf_sym);
-        if ( strcmp(sym_name, symbol) )
+        if ( elf_strcmp_safe(elf, sym_name, symbol) )
             continue;
         return sym;
     }