incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: Environment for the test of the string functions memory overrun with example
Date Thu, 13 Jul 2006 21:07:10 GMT
Farid Zaripov wrote:
>   I made the first version of the rw_alloc() / rw_free().
> 
>   Memory leak checking is not implemented.
> 
>   The files are attached.

Okay, this version is closer to what I have in mind :) There are
a few minor tweaks that I would still make but I think it's ready
to commit as is (please go ahead and have Anton do it). We can
make the tweaks later (we will also need a test to exercise this).

> 
[...]
> /************************************************************************
>  *
>  * rw_alloc.h - definitions of rw_alloc and rw_free
>  *
>  * $Id: rw_alloc.h
>  *
>  ***************************************************************************
>  *
>  * Copyright 2005-2006 The Apache Software Foundation or its licensors,
>  * as applicable.
>  *
>  * Copyright 2004-2006 Rogue Wave Software.

The copyright date should be 2006 (the year when the code was
written). The Rogue Wave copyright should be removed.

[...]
> #ifndef RW_ALLOC_H_INCLUDED
> #define RW_ALLOC_H_INCLUDED
> 
> 
> #include <testdefs.h>   // for test config macros
> #include <stddef.h>     // for size_t

Please use _RWSTD_SIZE_T instead of size_t and avoid #including
<stddef.h> or any other headers in all "public" headers (i.e.,
those that are visible to the tests). However, use size_t and
other standard names in driver source (.cpp) files.

[...]
> #include <assert.h>   // for assert()
> #include <stdlib.h>   // for atoi(), getenv()
> #include <string.h>   // for memset()
> #include <malloc.h>   // for malloc()

<malloc.h> is not standard/portable. Please #include <stdlib.h>
for malloc() and free().

> 
[...]
> static long getpagesize ()
> {
>     static long pagesize_ = 0;
> 
>     if (0 == pagesize_) {
>         SYSTEM_INFO info;
>         ::GetSystemInfo (&info);

FYI: there's usually no reason to qualify calls to global functions
with :: (we did this once, long time ago, to get around an MSVC 6
bug, but have since abandoned the hack).

[...]
> static DWORD translate_prot(int prot)
> {
>     static DWORD prots[8] = {

This should be declared const since it's not modified. Since the
function is trivial, it could be inlined. If you decide to inline
it be sure to move the static const array to file scope as some
compilers refuse to inline functions with static variables.

[...]
> static inline int mprotect(void *addr, size_t len, int prot)
> {
>     DWORD flOldProt;
>     return ::VirtualProtect (addr, len, translate_prot (prot),
>         &flOldProt) ? 0 : (errno = EINVAL, -1);

I would suggest to break up the complicated return statements
into separate statements for better readability.

[...]
> 
> struct BlockInfo
> {
>     bool   used_;   // true is block is used
>     void*  addr_;   // address of the allocated block

It's generally a good idea to lay out struct members from the
largest to the smallest so as to avoid waste due to padding.
Here, I would consider reserving a private bit in flags_ and
using it for the used_ status bit.

>     size_t size_;   // size of the allocated block
>     void*  data_;   // address of the user data
>     size_t udsz_;   // size of the user data
>     int    flags_;  // memory protection flags
> };
> 
[...]
>     MemRWGuard (void* addr, size_t size)
>     {
>         size_t pagesize = getpagesize ();
> 
>         size_t off = size_t (addr) % pagesize;

FYI, assuming pagesize is a power of 2 (it usually is), I believe
this could be written more efficiently like this:

           size_t off = size_t (addr) & (pagesize - 1);


[...]
> // allocate more blocks (allocates one memory page)
> static bool _rw_allocate_blocks()
> {
>     // count of the blocks per memory page
>     static size_t blocks_per_page = 0;
> 
>     size_t pagesize = getpagesize ();
> 
>     if (0 == blocks_per_page)
>         blocks_per_page = (pagesize - sizeof (Blocks)) / sizeof (BlockInfo) + 1;

Here it seems we might be able to make pagesize a static const
just like blocks_per_page and avoid calling getpagesize() each
time.

> 
[...]
> template <class Fun>
> static BlockInfo * _rw_find_if (Fun fun)

Let's avoid using templates in the test driver unless we have to
(e.g., unless maintaining separate versions of the code would be
a significant effort).

[...]
> _TEST_EXPORT void*
> rw_alloc(size_t nbytes, int flags/* = -1*/)
> {
>     static const int RWSTD_ALLOC_FLAGS = getenvflags ();
> 
>     // redefine flags if environment variable was set
>     if (0 != RWSTD_ALLOC_FLAGS)
>         flags = RWSTD_ALLOC_FLAGS;

I think we will want to override (some of) the bits only when
flags == -1. If the caller calls rw_alloc(RW_PROT_RDWR) we
certainly don't want to give back read-only memory :) For the
time being I can only envision wanting to be able to change
how the memory is allocated (i.e., malloc() vs. mmap()).

[...]
> _TEST_EXPORT void
> rw_free(void* addr)
> {
>     if (BlockInfo * info = _rw_find_by_addr (addr)) {

FWIW, this will be too slow as a general purpose allocator since
the find function is O(N). We'll need an O(lg N) algorithm to be
able to use it as a general allocator. That will require its own
separate data structure (such as a hash table).

Martin

Mime
View raw message