Re: [Bug-ddrescue] Alignment of I/O buffer for direct disk access could fail on some memory addresses?

[Heikki Tauriainen]

Hello Antonio,

On ma, 2016-01-04 at 20:16 +0100, Antonio Diaz Diaz wrote:
> 
> Heikki Tauriainen wrote:
> > 
> > I think it would be safer to compute "disp" using unsigned
> > arithmetic on an integral type large enough to hold the address of
> > the buffer.
> 
> The problem is that the C++ standard does not guarantee that such 
> integral type exists in all implementations. 'intptr_t/uintptr_t' are
> optional in C++11 and do not exist in C++03. (The alignment code of 
> ddrescue was written in 2005).

Indeed, it sadly seems near impossible to implement the arithmetic
operations used for the buffer alignment in a fully portable way using
only pure C++03 code.  (And as you mentioned, even uintptr_t is only
optional still in C++11...)

> I have released version 1.21-pre3 using a cast to 'unsigned long
> long', a type already used by ddrescue that most probably will be 
> able to contain the pointer value.

The unsigned type should in any case make it impossible to get a
negative value as a result of the cast operation (which I suspect might
have been the problem in my case).  Thanks for looking into this.

(As a matter of fact, on POSIX systems there appears to be the
posix_memalign(3) function available for allocating aligned memory, but
I'm not sure whether it could be used as a good enough replacement for
the current buffer alignment code as the function doesn't support
arbitrary alignment like the current implementation, and the function
is not specified in any C++ standard.  Then again, ddrescue already
appears to make assumptions that are beyond plain C++ standard
compliance as it makes use of library functions such as open(2) and
read(2)...)
> 
> > Alternatively, the program could (as a sanity check) abort with an
> > error in case the buffer cannot be adjusted to a multiple of the
> > sector size for direct disk access, or give more debug information
> > about the failing read(2) requests, instead of failing silently in
> > this case.
> 
> In a few days I'll add some code to make ddrescue exit on EINVAL and 
> report an error.
>
> Thanks again,
> Antonio.

I think that making ddrescue report such unexpected failures will be a
useful addition.  Thanks!

Regards,
Heikki