On Fri, Nov 13, 2015 at 02:08:59PM +0100, Jakub Hrozek wrote:
On Fri, Nov 13, 2015 at 01:40:16PM +0100, Sumit Bose wrote:
> > 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.
No, I think it's important to have good debug messages. I used the one
you suggested, there are no other changes in the patches.
...
+static int sbus_request_valist_check(va_list va, int first_arg_type)
+{
+ int ret = EOK;
+#ifdef HAVE_DBUSBASICVALUE
+ int type;
+ va_list va_check;
+ const DBusBasicValue *value;
+ bool ok;
+
+ va_copy(va_check, va);
+
+ type = first_arg_type;
+ while (type != DBUS_TYPE_INVALID) {
+ value = va_arg(va_check, const DBusBasicValue*);
+
+ if (type == DBUS_TYPE_STRING) {
+ ok = sss_utf8_check((const uint8_t *) value->str,
+ strlen(value->str));
+ if (!ok) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "Back end message [%s] contains invalid non-UTF8 " \
+ "characters");
value->str is missing, additionally sbus_request_return_and_finish() is
used by the responders as well so 'Back end message' should be changed
to 'S-Bus message' or similar.
bye,
Sumit
+ ret = EINVAL;
+ break;
+ }
+ }
+ type = va_arg(va_check, int);
+ }
+
+ va_end(va_check);
+#endif /* HAVE_DBUSBASICVALUE */
+ return ret;
+}
+