New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip'ing on a Source does not work #248

Open
noloader opened this Issue Aug 27, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@noloader
Collaborator

noloader commented Aug 27, 2016

The following was reported by user ahux on Stack Overflow at Skip'ing on a Source does not work:

I use Crypto++ 5.6.3 and I need the FileSource Skip(...) function. Unfortunately this function does nothing!

Here is a example for this function.

string filename = ...;
string str;

FileSource file(filename, false, new HexEncoder(new StringSink(str)));
file.Skip(24);
file.PumpAll();

Can somebody help me?


Below is the runnable test case using strings.

$ cat test.cxx
#include <string>
#include <iostream>
using namespace std;

#include <filters.h>
#include <hex.h>
using namespace CryptoPP;

int main(int argc, char* argv[])
{
  string str1, str2;
  HexEncoder enc(new StringSink(str1));
  for(unsigned int i=0; i < 32; i++)
    enc.Put((byte)i);
  enc.MessageEnd();

  cout << "str1: " << str1 <<endl;

  StringSource ss(str1, false, new StringSink(str2));
  ss.Skip(10);
  ss.PumpAll();

  cout << "str2: " << str2 << endl;

  return 0;
}

Here the [un]expected output:

$ ./test.exe
str1: 000102030405060708090A0B0C0D0E0F101112131415161718191A1B1C1D1E1F
str2: 000102030405060708090A0B0C0D0E0F101112131415161718191A1B1C1D1E1F
@noloader

This comment has been minimized.

Collaborator

noloader commented Aug 27, 2016

Looking at the documentation for BufferedTransformation::Skip:

lword BufferedTransformation::Skip (lword skipMax=LWORD_MAX)

Discard skipMax bytes from the output buffer.

Part of me wants to say, its working as expected because the operation is on the "output buffer" modulo pumpAll=false in StringSource ss(str1, false ...). There's nothing in the output buffer, which would be the AttachedTransformation(), so there's nothing to skip.

However, the other part of me says its not working as expected, regardless of the language lawyering.


As I step the code, I see exactly what's happening (the "working as expected" from above):

(gdb) b CryptoPP::Source::Skip
Breakpoint 1 at 0x40c120: file cryptlib.cpp, line 559.
(gdb) r 
Starting program: /home/jwalton/cryptopp/test.exe 
str1: 000102030405060708090A0B0C0D0E0F101112131415161718191A1B1C1D1E1F

Breakpoint 1, CryptoPP::BufferedTransformation::Skip (this=0x7fffffffd480, 
    skipMax=0xa) at cryptlib.cpp:559
559     if (AttachedTransformation())
(gdb) n
560         return AttachedTransformation()->Skip(skipMax);

I'm wondering if we should special case the Skip() for a Source. Maybe something like the following:

if (dynamic_cast<Source*>(this))
    dynamic_cast<Source*>(this)->SkipBytesBeingPumped(skipMax);
else
    if (AttachedTransformation())
        return AttachedTransformation()->Skip(skipMax);

The pain point seems to be SkipBytesBeingPumped. Each Source will need to implement something different based on the underlying type. A FileSource will have to move the pointer in the stream; a StringSource will have to maintain an index into the string; etc.

Maybe the way to approach this is to temporarily attach a discard filter, and Pump skipMax bytes into it. After skipMax is skipped, use the AttachedTransformation() as expected.

We have to be careful we don't break existing code in the field. This is worrisome because code in the field is a wild card we don't control.


The digested version of the analysis and open questions are:

  1. Is there an issue, and should we move on it?
  2. If we should move on it, then how should we move?
@zooko

This comment has been minimized.

Collaborator

zooko commented Aug 28, 2016

FYI, I've read this ticket and thought about it, but I don't know what the right answer is.

@noloader

This comment has been minimized.

Collaborator

noloader commented Sep 8, 2016

I think the way to go here is to provide a member function on Source that performs the operation. Some off-line conversations with DB and WD concluded the same.

The problem is, its so fundamental that it effects nearly every filter related class. Its a effectively a breaking change.

I'm going to stall on this issue at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment