linux-kernel - Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 3 Apr 2020 13:19:26 +0200
From:   Mauro Carvalho Chehab <mchehab@...nel.org>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Petr Mladek <pmladek@...e.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-media@...r.kernel.org,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        hverkuil@...all.nl,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joe Perches <joe@...ches.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>
Subject: Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and
 DRM fourccs

Em Fri, 3 Apr 2020 13:47:02 +0300
Sakari Ailus <sakari.ailus@...ux.intel.com> escreveu:

> > > +static noinline_for_stack
> > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > +		    struct printf_spec spec, const char *fmt)
> > > +{
> > > +#define FOURCC_STRING_BE	"-BE"
> > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > > +
> > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > +		return buf;
> > > +
> > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +		return error_string(buf, end, "(%p4?)", spec);
> > > +
> > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > > +
> > > +	if (*fourcc & BIT(31))
> > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > +			sizeof(FOURCC_STRING_BE));
> > > +
> > > +	return string(buf, end, s, spec);  
> > 
> > Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
> > (without quotes). There are other 4CCs that contain spaces and would
> > suffer from a similar issue. Even in little-endian format, it would
> > result in additional spaces in the output string. Is this what we want ?
> > Should the caller always enclose the 4CC in quotes or brackets for
> > clarity ? Or should still be done here ?  
> 
> Good question. Space is indeed a valid character in a 4cc code.
> 
> If I omit one or more spaces, it will no longer be a 4cc, but a 3cc or even
> a 2cc. Jokes aside, there are probably fair arguments both ways.
> 
> I presume there's no 4cc code where the location of a space would make a
> difference but all of the spaces are trailing spaces.

Yes. I guess it doesn't make any sense to allow a 4cc code with an
space before or in the middle.

Btw, on a quick search at the Internet for non-Linux definitions,
a Fourcc code "Y8  " is actually shown at the lists as just "Y8", 
e. g. removing the leading spaces:

	https://www.fourcc.org/codecs.php
	http://abcavi.kibi.ru/fourcc.php
	https://softron.zendesk.com/hc/en-us/articles/207695697-List-of-FourCC-codes-for-video-codecs
	https://www.free-codecs.com/guides/guides.php?f=fourcc

One interesting detail there is that some tables show some codes 
like "BGR(15)". While I'm not sure how this is encoded, I suspect
that the fourcc is actually "BGR\x15".

We don't do that on V4L, nor we have plans to do so. Not sure if
DRM would accept something like that. Of so, then the logic should
have some special handler if the code is below 32.	

> It's also worth noting that the formats printed are mostly for debugging
> purpose and thus even getting a hypothetical case wrong is not a grave
> issue. This would also support just printing them as-is though.
> 
> I'm leaning slightly towards omitting any spaces if the code has them. 

I would just remove trailing spaces, and then use a loop from the end
to remove trailing spaces (and optionally handle codes ending with a
value below 32, if are there any such case with DRM fourcc codes).

On the other hand, I don't mind if you prefer to use just one for()
loop and just trip any spaces inside it.

> This is something that couldn't be done by using a macro...

Well, I suspect that it might be possible to write a macro
for doing that too, for example using preprocessor concatenation
logic that could produce the same results. If you do something 
like that, however, I suspect that te macro would face some 
portability issues, as, as far as I know, not all C compilers
would handle string concatenation the same way.

Thanks,
Mauro