[Privoxy-devel] warning: unknown conversion type character 'l' in format

[Fabian Keil]

Lee <ler762 at gmail.com> wrote:

> On 6/7/17, Fabian Keil <fk at fabiankeil.de> wrote:
> > Lee <ler762 at gmail.com> wrote:
> >  
> >> GCC fusses about an %llu format and seems to translate it into %lu  
> >
> > What do you mean by "translate"?  
> 
> Rather than crashing or otherwise misbehaving, the program acts like
> it was given a format of "%lu"

If everything works it seems likely that the %llu is passed
to snprintf and dealt with properly. The compiler doesn't
have to understand or modify the format string for this to
work (but of course snprintf has to support the format
specifier).

> > Does the %llu not end up in the binary?  
> 
> I can grep for "%llu" in parsers.o & get "Binary file parsers.o
> matches" but I don't know how to tell if that specific "%llu" ends up
> in the binary or not :(

You could grep the strings output:

fk at t520 ~/git/privoxy $strings privoxy | grep  'Content-Length: %llu'
Content-Length: %llu

> >> ------- with %I64u
> >> 2017-06-05 14:05:40.360 00000d9c Request: www.installsite.org/
> >> 2017-06-05 14:05:40.578 00000d9c Info: buffer_and_filter_content set to 1
> >> 2017-06-05 14:05:40.578 00000d9c Info: create_content_length_header:
> >> Content-Length: 9855  
> >
> > If there was a problem with the %I64u it would be unlikely to
> > affect the log message as it doesn't print the "header" string.
> >
> > Errors are probably also more likely to show up with values
> > that can't be represented with 32 bit.  
> 
> Can you share a good reference for how C does type casting?

IIRC it's extensively covered in "The Art of Software Security Assessment"
by Dowd et al. and (probably) "Writing Secure Code" by Howard and LeBlanc
as well.

> Some of the WIN32 specific things like write_socket in jbsockets.c
> where it casts size_t len to (int)len for the send call seems like an
> error waiting to happen.  Unless len is somehow guaranteed never to be
> greater than 2^31-1??  ( size_t is unsigned, (int) is signed, so it
> seems to me that as long as the value of len fits in 31 bits
> everything will be fine.  What happens when the value of len is
> greater than 2^31?)

len is not expected to become that large because write_socket()
is used to write data from a buffer which is unlikely to become
that big.

I don't know how send() on Windows deals with negative lengths
or if Privoxy could be intentionally (mis)configured to result
in negative lengths getting passed to send(), but if you are
concerned about this you could enable the "#elif defined(__OS2__)"
block for WIN32 as well.

Maybe we should additionally add an artificial upper
buffer-limit limit or emit a warning if the chosen
value seems unwise.

> >> Can anyone think of a reason _not_ to add the
> >>
> >> #ifdef _WIN32
> >>    assert(sizeof(unsigned long long) > 4);
> >>    snprintf(header, buffer_length, "Content-Length: %I64u",
> >> content_length);
> >> #else
> >>
> >> bit to parsers.c::create_content_length_header ?  
> >
> > I have no objections to the snprint() with the modified
> > format specifier but one reason not to add the assertion
> > is that the condition can be checked at compile time.
> >
> > Probably we should replace the assertion I added in r1.209
> > with something like:
> >
> > commit bea35441b9f8e2bf78109544ebc54a17b8373cdd
> > Author: Fabian Keil <fk at fabiankeil.de>
> > Date:   Wed Jun 7 12:57:44 2017 +0200
> >
> >     get_content_length(): Replace an assertion with a compile-time check

I committed this.

> I don't know enough to have an informed opinion, so I'll go with
> whatever you think is best.
> 
> & just to be clear - you're ok with me applying this patch?
> 
> Index: current/parsers.c
[...]
>  {
> +#ifdef _WIN32
> +#if SIZEOF_LONG_LONG < 8
> +#error sizeof(unsigned long long) too small
> +#endif
> +   snprintf(header, buffer_length, "Content-Length: %I64u", content_length);
> +#else
>     snprintf(header, buffer_length, "Content-Length: %llu", content_length);
> +#endif
>  }

I'm okay with this.

Checking SIZEOF_LONG_LONG once (in get_content_length()) seems sufficient
to me, but if you are concerned that the other check could get lost in the
future the second check is fine with me as well.

Fabian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.privoxy.org/pipermail/privoxy-devel/attachments/20170608/7ac0d8ef/attachment.bin>