[SSSD] [PATCH] Don't pass user input as a printf format string argument

Jakub Hrozek jhrozek at redhat.com
Fri Jan 10 14:19:07 UTC 2014


On Fri, Jan 10, 2014 at 02:56:42PM +0100, Stef Walter wrote:
> On 10.01.2014 12:11, Jakub Hrozek wrote:
> > On Thu, Jan 09, 2014 at 09:32:00PM +0100, Stef Walter wrote:
> >> On 08.01.2014 23:27, Jakub Hrozek wrote:
> >>> On Wed, Jan 08, 2014 at 09:02:52PM +0100, Stef Walter wrote:
> >>>> On 08.01.2014 17:59, Simo Sorce wrote:
> >>>>> On Wed, 2014-01-08 at 11:21 +0100, Stef Walter wrote:
> >>>>>> On 07.01.2014 22:21, Simo Sorce wrote:
> >>>>>>> Sorry I forgot another, I think you should either set errno on errors,
> >>>>>>> or return an errno_t instead of -1. Just returning -1 for all errors is
> >>>>>>> a poor interface.
> >>>>>>
> >>>>>> It's the same interface as the functions being replaced: snprintf and
> >>>>>> asprintf. We return the full format length, or -1 on failure. snprintf
> >>>>>> and friends don't set errno. See man printf(3).
> >>>>>>
> >>>>>> But I guess now that we're diverging from the interface provided by
> >>>>>> snprintf and friends, I don't mind setting errno to EINVAL/ENOMEM if it
> >>>>>> makes you happy.
> >>>>>
> >>>>> Well I generally prefer to know if the format was wrong or ENOMEM
> >>>>> happened, but I see you removed the alloc version ... I guess in this
> >>>>> case I am not too picky, only format errors can happen now.
> >>>>
> >>>> I figured that neither neither realmd or sssd was using the asprintf
> >>>> variant (both used other allocation schemes) I might as well remove it.
> >>>
> >>> I agree it's better to not have code that's not used in the repo at all.
> >>> Such code can easily become subject to bitrot (it wouldn't be the
> >>> case with a utility function like this one perhaps, but still..)
> >>
> >> One more fix to these patches. I missed a couple signed compare problem
> >> problems in patch 0002. fq_len and nlen are now int.
> >>
> >> Stef
> >>
> > 
> > The patches look good to me and all the testing I did (various formats
> > of FQDN including invalid, no FQDN, groups with mixed-domain membership)
> > went fine.
> 
> Updated version which no longer conflicts with the .gitignore patch.
> 
> Stef

ACK again, I'm just putting the patch through Coverity before pushing.



More information about the sssd-devel mailing list