[v6,04/20] elf: Add _dl_audit_objsearch
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
It consolidates the code required to call la_objsearch() audit
callback.
Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu.
---
elf/dl-audit.c | 37 +++++++++++++++++++++++
elf/dl-load.c | 61 ++++----------------------------------
sysdeps/generic/ldsodefs.h | 2 ++
3 files changed, 44 insertions(+), 56 deletions(-)
Comments
* Adhemerval Zanella:
> diff --git a/elf/dl-audit.c b/elf/dl-audit.c
> index 5fbc76a36c..de85ef1ddd 100644
> --- a/elf/dl-audit.c
> +++ b/elf/dl-audit.c
> @@ -42,6 +42,43 @@ _dl_audit_activity_nsid (Lmid_t nsid, int action)
> _dl_audit_activity_map (head, action);
> }
>
> +bool
> +_dl_audit_objsearch (const char **name, const char **origname,
> + struct link_map *l, unsigned int code)
> +{
> + if (__glibc_likely (GLRO(dl_naudit) == 0)
> + || l == NULL || l->l_auditing
> + || code == 0)
> + return true;
> +
> + struct audit_ifaces *afct = GLRO(dl_audit);
> + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
> + {
> + if (afct->objsearch != NULL)
> + {
> + const char *before = *name;
> + struct auditstate *state = link_map_audit_state (l, cnt);
> + *name = afct->objsearch (*name, &state->cookie, code);
> + if (*name == NULL)
> + return false;
> +
> + if (origname != NULL && before != *name
> + && strcmp (before, *name) != 0)
> + {
> + if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
> + _dl_debug_printf ("audit changed filename %s -> %s\n",
> + before, *name);
> +
> + if (*origname == NULL)
> + *origname = before;
> + }
> + }
> + afct = afct->next;
> + }
> +
> + return true;
> +}
Sorry, I find the interface rather confusing. *name is an in-out
parameter, and *origname is an out parameter. This is not really clear
from their names.
I looked at the rest of elf/dl-load.c, and origname appears to be solely
used as an optimization, to avoid having to call add_name_to_object
twice. (add_name_to_object already filters out duplicate names.)
I think this interface should work:
const char *_dl_audit_objsearch (const char *name, struct link_map *l,
unsigned int code);
Callers need to check for NULL return values and stop loading the
object. Otherwise they have to register both names with
add_name_to_object.
What do you think?
Thanks,
Florian
On 17/12/2021 09:21, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> diff --git a/elf/dl-audit.c b/elf/dl-audit.c
>> index 5fbc76a36c..de85ef1ddd 100644
>> --- a/elf/dl-audit.c
>> +++ b/elf/dl-audit.c
>> @@ -42,6 +42,43 @@ _dl_audit_activity_nsid (Lmid_t nsid, int action)
>> _dl_audit_activity_map (head, action);
>> }
>>
>> +bool
>> +_dl_audit_objsearch (const char **name, const char **origname,
>> + struct link_map *l, unsigned int code)
>> +{
>> + if (__glibc_likely (GLRO(dl_naudit) == 0)
>> + || l == NULL || l->l_auditing
>> + || code == 0)
>> + return true;
>> +
>> + struct audit_ifaces *afct = GLRO(dl_audit);
>> + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>> + {
>> + if (afct->objsearch != NULL)
>> + {
>> + const char *before = *name;
>> + struct auditstate *state = link_map_audit_state (l, cnt);
>> + *name = afct->objsearch (*name, &state->cookie, code);
>> + if (*name == NULL)
>> + return false;
>> +
>> + if (origname != NULL && before != *name
>> + && strcmp (before, *name) != 0)
>> + {
>> + if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
>> + _dl_debug_printf ("audit changed filename %s -> %s\n",
>> + before, *name);
>> +
>> + if (*origname == NULL)
>> + *origname = before;
>> + }
>> + }
>> + afct = afct->next;
>> + }
>> +
>> + return true;
>> +}
>
> Sorry, I find the interface rather confusing. *name is an in-out
> parameter, and *origname is an out parameter. This is not really clear
> from their names.
>
> I looked at the rest of elf/dl-load.c, and origname appears to be solely
> used as an optimization, to avoid having to call add_name_to_object
> twice. (add_name_to_object already filters out duplicate names.)
>
> I think this interface should work:
>
> const char *_dl_audit_objsearch (const char *name, struct link_map *l,
> unsigned int code);
>
> Callers need to check for NULL return values and stop loading the
> object. Otherwise they have to register both names with
> add_name_to_object.
>
> What do you think?
To replace the second command and keep the 'origname' update we will
need to pass it as an out argument (or return as a tuple from
_dl_audit_objsearch).
Also, by just returning a 'const char *' there is no indication that
audit modules are really handled, so we need to check on both sites
if there are really enabled (which should be ok).
if (__glibc_unlikely (GLRO(dl_naudit) > 0))
{
[...]
}
What about:
const char *_dl_audit_objsearch (const char *name, const char **origname,
struct link_map *l, unsigned int code);
Where 'origname' is updated if non NULL?
* Adhemerval Zanella:
> To replace the second command and keep the 'origname' update we will
> need to pass it as an out argument (or return as a tuple from
> _dl_audit_objsearch).
Why is that? Isn't it going to be to the name variable in the caller?
Thanks,
Florian
On 17/12/2021 13:12, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> To replace the second command and keep the 'origname' update we will
>> need to pass it as an out argument (or return as a tuple from
>> _dl_audit_objsearch).
>
> Why is that? Isn't it going to be to the name variable in the caller?
>
> Thanks,
> Florian
>
Checking again it seems we can update it before, I will updated the patch.
@@ -42,6 +42,43 @@ _dl_audit_activity_nsid (Lmid_t nsid, int action)
_dl_audit_activity_map (head, action);
}
+bool
+_dl_audit_objsearch (const char **name, const char **origname,
+ struct link_map *l, unsigned int code)
+{
+ if (__glibc_likely (GLRO(dl_naudit) == 0)
+ || l == NULL || l->l_auditing
+ || code == 0)
+ return true;
+
+ struct audit_ifaces *afct = GLRO(dl_audit);
+ for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
+ {
+ if (afct->objsearch != NULL)
+ {
+ const char *before = *name;
+ struct auditstate *state = link_map_audit_state (l, cnt);
+ *name = afct->objsearch (*name, &state->cookie, code);
+ if (*name == NULL)
+ return false;
+
+ if (origname != NULL && before != *name
+ && strcmp (before, *name) != 0)
+ {
+ if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
+ _dl_debug_printf ("audit changed filename %s -> %s\n",
+ before, *name);
+
+ if (*origname == NULL)
+ *origname = before;
+ }
+ }
+ afct = afct->next;
+ }
+
+ return true;
+}
+
void
_dl_audit_objopen (struct link_map *l, Lmid_t nsid, bool check_audit)
{
@@ -1595,33 +1595,8 @@ open_verify (const char *name, int fd,
#ifdef SHARED
/* Give the auditing libraries a chance. */
- if (__glibc_unlikely (GLRO(dl_naudit) > 0) && whatcode != 0
- && loader->l_auditing == 0)
- {
- const char *original_name = name;
- struct audit_ifaces *afct = GLRO(dl_audit);
- for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
- {
- if (afct->objsearch != NULL)
- {
- struct auditstate *state = link_map_audit_state (loader, cnt);
- name = afct->objsearch (name, &state->cookie, whatcode);
- if (name == NULL)
- /* Ignore the path. */
- return -1;
- }
-
- afct = afct->next;
- }
-
- if (fd != -1 && name != original_name && strcmp (name, original_name))
- {
- /* An audit library changed what we're supposed to open,
- so FD no longer matches it. */
- __close_nocancel (fd);
- fd = -1;
- }
- }
+ if (!_dl_audit_objsearch (&name, NULL, loader, whatcode))
+ return -1;
#endif
if (fd == -1)
@@ -2059,36 +2034,10 @@ _dl_map_object (struct link_map *loader, const char *name,
#ifdef SHARED
/* Give the auditing libraries a chance to change the name before we
try anything. */
- if (__glibc_unlikely (GLRO(dl_naudit) > 0)
- && (loader == NULL || loader->l_auditing == 0))
+ if (!_dl_audit_objsearch (&name, &origname, loader, LA_SER_ORIG))
{
- struct audit_ifaces *afct = GLRO(dl_audit);
- for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
- {
- if (afct->objsearch != NULL)
- {
- const char *before = name;
- struct auditstate *state = link_map_audit_state (loader, cnt);
- name = afct->objsearch (name, &state->cookie, LA_SER_ORIG);
- if (name == NULL)
- {
- /* Do not try anything further. */
- fd = -1;
- goto no_file;
- }
- if (before != name && strcmp (before, name) != 0)
- {
- if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
- _dl_debug_printf ("audit changed filename %s -> %s\n",
- before, name);
-
- if (origname == NULL)
- origname = before;
- }
- }
-
- afct = afct->next;
- }
+ fd = -1;
+ goto no_file;
}
#endif
@@ -1392,6 +1392,8 @@ link_map_audit_state (struct link_map *l, size_t index)
}
}
+bool _dl_audit_objsearch (const char **name, const char **origname,
+ struct link_map *l, unsigned int code);
/* Call the la_activity() from the audit modules from the link map L
and issues the ACTION argument. */
void _dl_audit_activity_map (struct link_map *l, int action)