Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aya-log panics when logging [u8;6] without format argument #513

Closed
arctic-alpaca opened this issue Feb 2, 2023 · 7 comments · Fixed by #606
Closed

aya-log panics when logging [u8;6] without format argument #513

arctic-alpaca opened this issue Feb 2, 2023 · 7 comments · Fixed by #606
Assignees
Labels
aya-log Relating to aya-log bug Something isn't working
Milestone

Comments

@arctic-alpaca
Copy link
Contributor

arctic-alpaca commented Feb 2, 2023

Using aya-log to log a [u8;6] compiles and leads to a runtime panic if no format argument (e.g. :mac) is provided.

Example:
info!(&ctx, "received a packet: {}", [0_u8;6]);

Error:
thread 'tokio-runtime-worker' panicked at 'called `Result::unwrap()` on an `Err` value: ()', /home/arctic-alpaca/.cargo/registry/src/github.com-1ecc6299db9ec823/aya-log-0.1.13/src/lib.rs:110:45

Used a newly created xdp project from the template to reproduce.

@alessandrod
Copy link
Collaborator

@vadorovsky when you get a chance could you look into this?

@vadorovsky vadorovsky self-assigned this Feb 13, 2023
@alessandrod alessandrod added this to the March Release 🗓️ milestone Feb 23, 2023
@dave-tucker dave-tucker added bug Something isn't working aya-log Relating to aya-log labels Feb 23, 2023
@Hanaasagi
Copy link
Contributor

@alessandrod @vadorovsky Hi, I think I understand why the error was thrown. Firstly, the write function is implemented for the [u8; 6] type, as shown here:

impl WriteToBuf for [u8; 6] {
fn write(&self, buf: &mut [u8]) -> Result<usize, ()> {
TagLenValue::<Argument>::new(Argument::ArrU8Len6, self).write(buf)
}
}

This writes the type of the field and some other information, which is then retrieved when formatting the string, as shown here:

aya/aya-log/src/lib.rs

Lines 463 to 465 in 23ce42d

Argument::ArrU8Len6 => {
let value: [u8; 6] = attr.value.try_into().map_err(|_| ())?;
full_log_msg.push_str(&value.format(last_hint.take())?);

Because {} is used in info!(&ctx, "received a packet: {}", [0_u8;6]);, the last_hint variable is None, and an error is thrown when formatting, as shown here:

aya/aya-log/src/lib.rs

Lines 217 to 231 in 23ce42d

impl Format for [u8; 6] {
fn format(&self, last_hint: Option<DisplayHint>) -> Result<String, ()> {
match last_hint {
Some(DisplayHint::Default) => Err(()),
Some(DisplayHint::LowerHex) => Err(()),
Some(DisplayHint::UpperHex) => Err(()),
Some(DisplayHint::Ipv4) => Err(()),
Some(DisplayHint::Ipv6) => Err(()),
Some(DisplayHint::LowerMac) => Ok(LowerMacFormatter::format(*self)),
Some(DisplayHint::UpperMac) => Ok(UpperMacFormatter::format(*self)),
_ => Err(()),
}
}
}

And currently, it seems that only {:mac} or {:MAC} can be used for output. I think we could add a DebugFormatter with {:?} format syntax or modify the DefaultFormatter to support this.

@Hanaasagi
Copy link
Contributor

Please give me some feedback. If it would be helpful, I would be happy to take on the responsibility of addressing this issue.

@alessandrod
Copy link
Collaborator

I think the issue is here

log_buf(buf, &*log).unwrap();
? We shouldn't unwrap

@Hanaasagi
Copy link
Contributor

Indeed, unwrap is not appropriate here, but it is not the root cause of this bug because Err has already occurred. As I mentioned in my previous response, the error thrown by info!(&ctx, "received a packet: {}", [0_u8;6]); is due to the lack of a defined DefaultFormatter processing logic for the [u8;6] type. Therefore, I think resolving this issue depends on whether code like info!("{}", [0_u8;6]) is supported.

  • If it is valid, then a DefaultFormatter should be added for the [u8;6] type, for example:
impl Formatter<[u8; 6]> for DefaultFormatter; {
    fn format(v: [u8; 6]) -> String {
        // e.g.
        format!("{:?}", v)
    }
}
  • If it is invalid, then the timing of throwing the error should be moved to the proc macro. The error should be thrown during compile time rather than runtime.

Here is an example code that moves the error timing to compile time:

main...Hanaasagi:aya:example

@vadorovsky
Copy link
Member

@Hanaasagi that's right, thanks for debugging that!

I think we shouldn't support info!("{}", [0_u8;6]) - std formatter in Rust doesn't support it either:

error[[E0277]](https://doc.rust-lang.org/stable/error_codes/E0277.html): `[u8; 6]` doesn't implement `std::fmt::Display`
 --> src/main.rs:2:20
  |
2 |     println!("{}", [0u8; 6]);
  |                    ^^^^^^^^ `[u8; 6]` cannot be formatted with the default formatter

So I like your second option - moving the error to proc macro. Happy to see that you started working on it! Would you like to submit a pull request?

@Hanaasagi
Copy link
Contributor

Sure, I will open a pull request to validate this in proc macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya-log Relating to aya-log bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants