search

inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
226 stars 39 forks source link

Request: add SearchEntry::try_construct that doesn't panic #135

Open feliwir opened 1 week ago

feliwir commented 1 week ago

Hey, right now i'm having issues with SearchEntry::construct, since it can cause crashes. Is there a "crash-safe" variant i can use somewhere? Are there any plans to add such a function?

Thanks!

inejge commented 1 week ago

To answer your questions first, a) there is no non-panicking entry parser, b) introducing error handling would be a pretty massive overhaul, so I don't have any immediate plans to do so. Long term, maybe.

What's the use case where message/entry parsing errors are a realistic problem? In all the years I've been working on the crate, #46 was the only time I've got a report of server software causing a panic.

feliwir commented 1 week ago

The issue is that we rely on our software to be somewhat "crash-resistant". Relying on the LDAP server sending a "valid" response is not really an acceptable risk.

Instead of "proper" error handling couldn't you just return a Box<dyn std::error::Error> and replace the .expect calls with ?. The error would just be propagated to the function call. If that is an acceptable path for you I'd be willing to create a PR.

inejge commented 1 week ago

Relying on the LDAP server sending a "valid" response is not really an acceptable risk.

It has been acceptable, where "acceptable" stands for "will panic for malformed input", for the users of the crate so far -- which is why I'm curious about the environment where that is insufficient.

Instead of "proper" error handling couldn't you just return a Box and replace the .expect calls with ?. The error would just be propagated to the function call.

That is proper error handling: the library itself can't do anything else with parsing errors (aside from panicking, as is the case now.) The problem is that constructing an entry is far from the only place where the parsing takes place. Message reception and control/exop construction can also panic, and I wouldn't accept a PR which doesn't handle those. I would also insist on keeping the panicking methods to keep the API stable.

I'm aware that not propagating the errors is less robust than doing so, and I'm not completely against improving the error handling, but I don't see it as a priority, absent a better motivation.

feliwir commented 1 week ago

It has been acceptable, where "acceptable" stands for "will panic for malformed input", for the users of the crate so far -- which is why I'm curious about the environment where that is insufficient.

We are a medical device manufacturer and our software will be / is used in critical infrastructure (hospitals etc.). Of course we could just "restart" our software every time we crash, but handling errors in a different manner is preferred.

I would also insist on keeping the panicking methods to keep the API stable.

Yes, i agree 100% with that. Looking at the stdlib and other crates i suggest using the try_X prefix for the new methods.