On Wed, Nov 11, 2015 at 12:00:31PM +0100, Jakub Hrozek wrote:
On Mon, Nov 09, 2015 at 03:14:31PM +0100, Jakub Hrozek wrote:
> On Mon, Nov 09, 2015 at 12:44:17PM +0100, Sumit Bose wrote:
> > On Fri, Nov 06, 2015 at 09:32:23PM +0100, Jakub Hrozek wrote:
> > > On Fri, Nov 06, 2015 at 09:13:07PM +0100, Jakub Hrozek wrote:
> > > > On Fri, Nov 06, 2015 at 04:59:58PM +0100, Sumit Bose wrote:
> > > > > > @@ -658,11 +678,7 @@ static void
get_subdomains_callback(struct be_req *req,
> > > > > > */
> > > > > > err_maj = dp_err_type;
> > > > > > err_min = errnum;
> > > > > > - if (errstr) {
> > > > > > - err_msg = errstr;
> > > > > > - } else {
> > > > > > - err_msg = dp_err_to_string(dp_err_type);
> > > > > > - }
> > > > > > + err_msg = safe_be_req_err_msg(errstr,
dp_err_type);
> > > > >
> > > > > Wouldn't it be even more safe to do this check in
> > > > > sbus_request_return_and_finish() and not rely on the callers?
> > > >
> > > > Something like the attached patch? I think we should check in
> > > > sbus_request_return_and_finish() in addition to checking in the
callers,
> > > > not instead, because the string in the callback is just a
diagnostics
> > > > message and we can recover by using some fallback.
> > > >
> > > > If we get all the way to sbus_request_return_and_finish(), then
failing
> > > > gracefully is better then abort() that libdbus calls, but it's
even better
> > > > to finish the request with wrong error message then fail completely.
> > > >
> > > > (I also still dislike that libdbus aborts...)
> > >
> > > Also, self-nack. dbus_validate_utf8() is not available on RHEL-6:
> > >
http://sssd-ci.duckdns.org/logs/job/32/17/rhel6/ci-build-debug/ci-make-te...
> > >
> > > I guess we can limit these checks to new OSs or use sss_utf8_check() on
> > > older platforms..
> >
> > Why not always use sss_utf8_check()? Is there an advantage using
> > dbus_validate_utf8()?
>
> I don't know how dbus_validate_utf8() works internally, so I tried to
> stick to the dbus-provided version to check dbus types.
>
> But if both do the same thing (If they don't it would be a bug, sure),
> we can stick to sss_utf8_check()
>
> (Another advantage might be that we could get away with linking against
> libdbus only)
Hi,
attached are patches that only use sss_utf8_check() and work on RHEL-6
also.
The patches look good and fixing the issue as expected. CI passes as
well
http://sssd-ci.duckdns.org/logs/job/33/16/summary.html (the RHEL6
failure is unrelated). ACK
I just have a minor comment
+static const char *safe_be_req_err_msg(const char *msg_in,
+ int dp_err_type)
+{
+ bool ok;
+
+ if (msg_in == NULL) {
+ /* No custom error, just use default */
+ return dp_err_to_string(dp_err_type);
+ }
+
+ ok = sss_utf8_check((const uint8_t *) msg_in,
+ strlen(msg_in));
+ if (!ok) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "Back end message is invalid, using default\n");
Maybe something like
DEBUG(SSSDBG_MINOR_FAILURE,
"Back end message [%s] contains invalid non-UTF8 character, " \
"using default\n", msg_in);
would be more specific and would help to better identify the source of the
message as well? Similar for sbus_request_valist_check().
But to fix the issue at hand and not to delay the release this can of
course be added later as well.
bye,
Sumit
+ return dp_err_to_string(dp_err_type);
+ }
+
+ return msg_in;
+}
+