[Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Paweł Bylica-2
Hi,

Recently I was hit by an assert in WinCOFFObjectWriter that had forbidden storing pointer to string table in header name field when the pointer had more that 6 decimal digits. This limit had been chosen to make implementation easier (sprintf adds null character at the end) and could be increased to 7 digits.

My patch is attached. The implementation uses additional buffer on the stack to make integer to string conversion.

- Paweł Bylica

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

WinCOFFObjectWriter_PointerToStingTable.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Nico Rieck
On 23.07.2013 18:29, Paweł Bylica wrote:
> Hi,
>
> Recently I was hit by an assert in WinCOFFObjectWriter that had forbidden
> storing pointer to string table in header name field when the pointer had
> more that 6 decimal digits. This limit had been chosen to make
> implementation easier (sprintf adds null character at the end) and could be
> increased to 7 digits.

I've already implemented this (and also included the undocumented base64
encoding), see: http://llvm-reviews.chandlerc.com/D667

Maybe someone actually hitting this limit in practice will make someone
accept at least the first part of D667.

-Nico

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Reid Kleckner-2
In reply to this post by Paweł Bylica-2
Is there a problem if the string is not null terminated?  If not, you can snprintf it right into place instead of doing sprintf+mempcy.


On Tue, Jul 23, 2013 at 12:29 PM, Paweł Bylica <[hidden email]> wrote:
Hi,

Recently I was hit by an assert in WinCOFFObjectWriter that had forbidden storing pointer to string table in header name field when the pointer had more that 6 decimal digits. This limit had been chosen to make implementation easier (sprintf adds null character at the end) and could be increased to 7 digits.

My patch is attached. The implementation uses additional buffer on the stack to make integer to string conversion.

- Paweł Bylica

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Nico Rieck
On 23.07.2013 18:43, Reid Kleckner wrote:
> Is there a problem if the string is not null terminated?  If not, you can
> snprintf it right into place instead of doing sprintf+mempcy.

snprintf always null-terminates (and truncates if there's not enough space).

-Nico

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Nico Rieck
On 23.07.2013 18:48, Nico Rieck wrote:> On 23.07.2013 18:43, Reid
Kleckner wrote:
 >> Is there a problem if the string is not null terminated?  If not,
you can
 >> snprintf it right into place instead of doing sprintf+mempcy.
 >
 > snprintf always null-terminates (and truncates if there's not enough
 > space).

Urgh, nevermind. Brain fart here.

-Nico

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Reid Kleckner-2
In reply to this post by Nico Rieck
On Tue, Jul 23, 2013 at 12:48 PM, Nico Rieck <[hidden email]> wrote:
On 23.07.2013 18:43, Reid Kleckner wrote:
Is there a problem if the string is not null terminated?  If not, you can
snprintf it right into place instead of doing sprintf+mempcy.

snprintf always null-terminates (and truncates if there's not enough space).

Nuh uh: "The _snprintf function formats and stores count or fewer characters in buffer, and appends a terminating null character if the formatted string length is strictly less than count characters."

Please don't assume snprintf always null terminates.

This may be Windows-specific behavior that you shouldn't rely on.  If that's the case, ignore my suggestion.

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Nico Rieck
On 23.07.2013 18:55, Reid Kleckner wrote:
> Please don't assume snprintf always null terminates.

I'm reading C99 7.19.6.4 and C11 7.21.6.5 which says:

"If n is zero, nothing is written, and s may  be  a  null  pointer.
Otherwise,  output  characters  beyond  the n-1st  are discarded rather
than being written to the array, and a null character is written at the
end of the characters actually written into the array."

and "Thus,  the  null-terminated  output  has  been
completely written if and only if the returned value is nonnegative and
less than n."

-Nico
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Richard-45
In reply to this post by Reid Kleckner-2

In article <CACs=[hidden email]>,
    Reid Kleckner <[hidden email]> writes:

> Is there a problem if the string is not null terminated?  If not, you can
> snprintf it right into place instead of doing sprintf+mempcy.

Am I the only one who scratches my head and says:

        sprintf?
        memcpy?

Why are we using error-prone C APIs in C++ code?
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
     The Computer Graphics Museum <http://computergraphicsmuseum.org>
         The Terminals Wiki <http://terminals.classiccmp.org>
  Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Richard-45
In reply to this post by Reid Kleckner-2

In article <CACs=[hidden email]>,
    Reid Kleckner <[hidden email]> writes:

> Nuh uh: "The _snprintf function formats and stores count or fewer
> characters in buffer, and appends a terminating null character if the
> formatted string length is strictly less than count characters."
> http://msdn.microsoft.com/en-us/library/2ts7cx93(v=vs.100).aspx
>
> Please don't assume snprintf always null terminates.
>
> This may be Windows-specific behavior that you shouldn't rely on.  If
> that's the case, ignore my suggestion.

Yes, _snprintf with MSVC is not a conforming C99 (or whatever) standard
implementation of snprintf.  I've filed a bug on it, but don't expect
a fix from them anytime soon as they deem it low priority.

This is just another reason to stay away from these error-prone C APIs
in C++ code.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
     The Computer Graphics Museum <http://computergraphicsmuseum.org>
         The Terminals Wiki <http://terminals.classiccmp.org>
  Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] WinCOFFObjectWriter: fix for storing pointer to string table in header name field

Paweł Bylica
In reply to this post by Richard-45



On Tue, Jul 23, 2013 at 8:36 PM, Richard <[hidden email]> wrote:

In article <CACs=[hidden email]>,
    Reid Kleckner <[hidden email]> writes:

> Is there a problem if the string is not null terminated?  If not, you can
> snprintf it right into place instead of doing sprintf+mempcy.

Am I the only one who scratches my head and says:

        sprintf?
        memcpy?

Why are we using error-prone C APIs in C++ code?
 
I've just kept the style of the previous implementation. I'd love to use std::to_string(), but it is just not available. Moreover, you still have to move this 8 bytes of text somehow.


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev