aya-obj: Handle lack of match of enum variants correctly #874

vadorovsky · 2024-01-25T20:34:19Z

When comparing local_spec with target_spec for enum relocations, we can encounter a sitiation when a matchinng variant in a candidate spec doesn't exist.

Before this change, such case wasn't handled explicitly, therefore resulted in returning currently constructed target_spec at the end. The problem is that such target_spec was, due to lack of match, incomplete. It didn't contain any accessors nor parts.

Later usage of such incomplete target_spec was leading to panics, since the code operating on enums' target_spec expects at least one accessor to be available.

Fixes: #868

This change is

netlify · 2024-01-25T20:34:24Z

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`1af9e5a`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/65b776b2272ea900086e82bb
😎 Deploy Preview	https://deploy-preview-874--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tamird

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @alessandrod and @vadorovsky)

-- commits line 5 at r1:
situation

-- commits line 17 at r1:
you can omit the colon

aya-obj/src/btf/relocation.rs line 490 at r1 (raw file):

                }
                BtfType::Enum64(en) => {
                    for (index, member) in en.variants.iter().enumerate() {

can we write this as return Ok(en.variants.iter().enumerate().find(....))? ditto above

even better would be to remove the part of this function that's outside the match to prevent this sort of mistake.

vadorovsky

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @alessandrod and @tamird)

-- commits line 5 at r1:

Previously, tamird (Tamir Duberstein) wrote…

situation

Done.

-- commits line 17 at r1:

Previously, tamird (Tamir Duberstein) wrote…

you can omit the colon

Done.

aya-obj/src/btf/relocation.rs line 490 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can we write this as return Ok(en.variants.iter().enumerate().find(....))? ditto above

even better would be to remove the part of this function that's outside the match to prevent this sort of mistake.

Done, I did the latter as well - now the whole match serves as a returned value.

I was thinking whether I can somehow improve the last match arm and avoid early returns there, but I think it's fine as it is (Ok(target_spec) and the end of field relocation arm and early return Ok(None) in case something goes wrong) - this logic seems to work for struct fields.

tamird

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @vadorovsky)

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

                    .find(|(index, member)| {
                        match_enum(member.name_offset, *index, &mut target_spec)
                            .is_ok_and(|m| m.is_some())

consider a match here. things like is_ok and is_some obscure what's being dropped. I see there was already a bug here at one point.

vadorovsky

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @alessandrod and @tamird)

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider a match here. things like is_ok and is_some obscure what's being dropped. I see there was already a bug here at one point.

I'm using matches!now - clippy complained about explicit match, but lmk if you rather keep the match explicit and annotate it with #[allow(clippy::match_like_matches_macro)].

tamird

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @vadorovsky)

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

I'm using matches!now - clippy complained about explicit match, but lmk if you rather keep the match explicit and annotate it with #[allow(clippy::match_like_matches_macro)].

The problem is still here: if you use _ anywhere without type ascription then you don't know the type of what you're throwing away

vadorovsky · 2024-01-29T07:51:07Z

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

The problem is still here: if you use _ anywhere without type ascription then you don't know the type of what you're throwing away

I noticed one more problem - the match_enum closure returns an error, but we were always ignoring it (both in the old code and my previous push). Also, returning Some(()) there is kinda confusing.

In retrospect, I pushed a new solution where:

I'm returning Result<bool, BtfError> in the match_enum closure.
Because of having not only bool, but Result<bool> as the type, and because I want to early return on the error, I don't think using find() makes sense, so I went back to using a loop. I was thinking about try_for_eachor try_fold, but I feel like that would be less readable. I think it's just cleaner and less confusing to use a loop in that case, but feel free to disagree if you have something in mind (I would appreciate a snippet).

tamird

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @vadorovsky)

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

I noticed one more problem - the match_enum closure returns an error, but we were always ignoring it (both in the old code and my previous push). Also, returning Some(()) there is kinda confusing.

In retrospect, I pushed a new solution where:

I'm returning Result<bool, BtfError> in the match_enum closure.

Because of having not only bool, but Result<bool> as the type, and because I want to early return on the error, I don't think using find() makes sense, so I went back to using a loop. I was thinking about try_for_eachor try_fold, but I feel like that would be less readable. I think it's just cleaner and less confusing to use a loop in that case, but feel free to disagree if you have something in mind (I would appreciate a snippet).

                BtfType::Enum64(en) => {
                  let enum_match = en
                    .variants
                    .iter()
                    .enumerate()
                    .find(|(index, member)| {
                            !matches!(match_enum(member.name_offset, *index, &mut target_spec), Ok(true))
                    }).transpose()?;
                  enum_match.then_some(target_spen)

How does that look to you?

tamird

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @vadorovsky)

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

                BtfType::Enum64(en) => {
                  let enum_match = en
                    .variants
                    .iter()
                    .enumerate()
                    .find(|(index, member)| {
                            !matches!(match_enum(member.name_offset, *index, &mut target_spec), Ok(true))
                    }).transpose()?;
                  enum_match.then_some(target_spen)

How does that look to you?

Oops, that should be !matches!(match_enum(member.name_offset, *index, &mut target_spec), Ok(false))

Another idea is to move more logic into match_enum.

diff --git a/aya-obj/src/btf/relocation.rs b/aya-obj/src/btf/relocation.rs
index c23bee7..0068652 100644
--- a/aya-obj/src/btf/relocation.rs
+++ b/aya-obj/src/btf/relocation.rs
@@ -460,8 +460,14 @@ fn match_candidate<'target>(
             let target_ty = candidate.btf.type_by_id(target_id)?;
             // the first accessor is guaranteed to have a name by construction
             let local_variant_name = local_spec.accessors[0].name.as_ref().unwrap();
-            let match_enum =
-                |name_offset, index, target_spec: &mut AccessSpec| -> Result<_, BtfError> {
+            fn match_enum<'a>(
+                iterator: impl Iterator<Item = (usize, u32)>,
+                candidate: &Candidate,
+                local_variant_name: &str,
+                target_id: u32,
+                mut target_spec: AccessSpec<'a>,
+            ) -> Result<Option<AccessSpec<'a>>, BtfError> {
+                for (index, name_offset) in iterator {
                     let target_variant_name = candidate.btf.string_at(name_offset)?;
                     if flavorless_name(local_variant_name) == flavorless_name(&target_variant_name)
                     {
@@ -471,27 +477,35 @@ fn match_candidate<'target>(
                             type_id: target_id,
                             name: None,
                         });
-                        Ok(Some(()))
-                    } else {
+                        return Ok(Some(target_spec));
+                    }
+                }
                 Ok(None)
             }
-                };
             match target_ty {
                 BtfType::Enum(en) => {
-                    for (index, member) in en.variants.iter().enumerate() {
-                        if let Ok(Some(_)) = match_enum(member.name_offset, index, &mut target_spec)
-                        {
-                            return Ok(Some(target_spec));
-                        }
-                    }
+                    return match_enum(
+                        en.variants
+                            .iter()
+                            .map(|member| member.name_offset)
+                            .enumerate(),
+                        candidate,
+                        local_variant_name,
+                        target_id,
+                        target_spec,
+                    ).map_err(Into::into)
                 }
                 BtfType::Enum64(en) => {
-                    for (index, member) in en.variants.iter().enumerate() {
-                        if let Ok(Some(_)) = match_enum(member.name_offset, index, &mut target_spec)
-                        {
-                            return Ok(Some(target_spec));
-                        }
-                    }
+                    return match_enum(
+                        en.variants
+                            .iter()
+                            .map(|member| member.name_offset)
+                            .enumerate(),
+                        candidate,
+                        local_variant_name,
+                        target_id,
+                        target_spec,
+                    ).map_err(Into::into)
                 }
                 _ => return Ok(None),
             }

vadorovsky · 2024-01-29T09:48:04Z

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Oops, that should be !matches!(match_enum(member.name_offset, *index, &mut target_spec), Ok(false))

Another idea is to move more logic into match_enum.

diff --git a/aya-obj/src/btf/relocation.rs b/aya-obj/src/btf/relocation.rs
index c23bee7..0068652 100644
--- a/aya-obj/src/btf/relocation.rs
+++ b/aya-obj/src/btf/relocation.rs
@@ -460,8 +460,14 @@ fn match_candidate<'target>(
             let target_ty = candidate.btf.type_by_id(target_id)?;
             // the first accessor is guaranteed to have a name by construction
             let local_variant_name = local_spec.accessors[0].name.as_ref().unwrap();
-            let match_enum =
-                |name_offset, index, target_spec: &mut AccessSpec| -> Result<_, BtfError> {
+            fn match_enum<'a>(
+                iterator: impl Iterator<Item = (usize, u32)>,
+                candidate: &Candidate,
+                local_variant_name: &str,
+                target_id: u32,
+                mut target_spec: AccessSpec<'a>,
+            ) -> Result<Option<AccessSpec<'a>>, BtfError> {
+                for (index, name_offset) in iterator {
                     let target_variant_name = candidate.btf.string_at(name_offset)?;
                     if flavorless_name(local_variant_name) == flavorless_name(&target_variant_name)
                     {
@@ -471,27 +477,35 @@ fn match_candidate<'target>(
                             type_id: target_id,
                             name: None,
                         });
-                        Ok(Some(()))
-                    } else {
+                        return Ok(Some(target_spec));
+                    }
+                }
                 Ok(None)
             }
-                };
             match target_ty {
                 BtfType::Enum(en) => {
-                    for (index, member) in en.variants.iter().enumerate() {
-                        if let Ok(Some(_)) = match_enum(member.name_offset, index, &mut target_spec)
-                        {
-                            return Ok(Some(target_spec));
-                        }
-                    }
+                    return match_enum(
+                        en.variants
+                            .iter()
+                            .map(|member| member.name_offset)
+                            .enumerate(),
+                        candidate,
+                        local_variant_name,
+                        target_id,
+                        target_spec,
+                    ).map_err(Into::into)
                 }
                 BtfType::Enum64(en) => {
-                    for (index, member) in en.variants.iter().enumerate() {
-                        if let Ok(Some(_)) = match_enum(member.name_offset, index, &mut target_spec)
-                        {
-                            return Ok(Some(target_spec));
-                        }
-                    }
+                    return match_enum(
+                        en.variants
+                            .iter()
+                            .map(|member| member.name_offset)
+                            .enumerate(),
+                        candidate,
+                        local_variant_name,
+                        target_id,
+                        target_spec,
+                    ).map_err(Into::into)
                 }
                 _ => return Ok(None),
             }

Done. I like the second solution. I applied it (the only thing I changed is not using the reduntant return in BpfType:: arms).

tamird

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @vadorovsky)

aya-obj/src/btf/relocation.rs line 486 at r3 (raw file):

Previously, vadorovsky (Michal Rostecki) wrote…

Done. I like the second solution. I applied it (the only thing I changed is not using the reduntant return in BpfType:: arms).

Yeah, I had the return because I was working against the diff base. I think you can drop the .map_err(Into::into), calls if you change the return type of match_enum to Result<Option<AccessSpec<'a>>, RelocationError>

When comparing `local_spec` with `target_spec` for enum relocations, we can encounter a situation when a matchinng variant in a candidate spec doesn't exist. Before this change, such case wasn't handled explicitly, therefore resulted in returning currently constructed `target_spec` at the end. The problem is that such `target_spec` was, due to lack of match, incomplete. It didn't contain any `accessors` nor `parts`. Later usage of such incomplete `target_spec` was leading to panics, since the code operating on enums' `target_spec` expects at least one `accessor` to be available. Fixes aya-rs#868

tamird

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)

mergify bot added aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI labels Jan 25, 2024

vadorovsky force-pushed the core-enum branch from 307b34e to 4c536c8 Compare January 25, 2024 20:36

vadorovsky requested review from alessandrod and tamird January 25, 2024 20:44

tamird requested changes Jan 25, 2024

View reviewed changes

vadorovsky force-pushed the core-enum branch from 4c536c8 to cc23afb Compare January 26, 2024 10:37

vadorovsky commented Jan 26, 2024

View reviewed changes

vadorovsky force-pushed the core-enum branch from cc23afb to 3de054c Compare January 26, 2024 10:47

tamird requested changes Jan 26, 2024

View reviewed changes

vadorovsky force-pushed the core-enum branch 2 times, most recently from eda0bf3 to f1b6a40 Compare January 26, 2024 16:23

vadorovsky commented Jan 26, 2024

View reviewed changes

tamird requested changes Jan 26, 2024

View reviewed changes

vadorovsky force-pushed the core-enum branch from f1b6a40 to 2fc9b4b Compare January 29, 2024 07:51

tamird requested changes Jan 29, 2024

View reviewed changes

vadorovsky force-pushed the core-enum branch from 2fc9b4b to f57c7b6 Compare January 29, 2024 09:46

tamird approved these changes Jan 29, 2024

View reviewed changes

vadorovsky force-pushed the core-enum branch from f57c7b6 to 1af9e5a Compare January 29, 2024 09:58

tamird approved these changes Jan 29, 2024

View reviewed changes

vadorovsky merged commit c05a3b6 into aya-rs:main Jan 29, 2024
21 checks passed

vadorovsky deleted the core-enum branch January 29, 2024 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aya-obj: Handle lack of match of enum variants correctly #874

aya-obj: Handle lack of match of enum variants correctly #874

vadorovsky commented Jan 25, 2024 •

edited

Loading

netlify bot commented Jan 25, 2024 •

edited

Loading

tamird left a comment

vadorovsky left a comment

tamird left a comment

vadorovsky left a comment

tamird left a comment

vadorovsky commented Jan 29, 2024

tamird left a comment

tamird left a comment

vadorovsky commented Jan 29, 2024

tamird left a comment

tamird left a comment

aya-obj: Handle lack of match of enum variants correctly #874

aya-obj: Handle lack of match of enum variants correctly #874

Conversation

vadorovsky commented Jan 25, 2024 • edited Loading

netlify bot commented Jan 25, 2024 • edited Loading

✅ Deploy Preview for aya-rs-docs ready!

tamird left a comment

Choose a reason for hiding this comment

vadorovsky left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

vadorovsky left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

vadorovsky commented Jan 29, 2024

tamird left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

vadorovsky commented Jan 29, 2024

tamird left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

vadorovsky commented Jan 25, 2024 •

edited

Loading

netlify bot commented Jan 25, 2024 •

edited

Loading