-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hello again,
The attached patch should rework the function copy constructors. Best regards, Adam. On 14.11.2012 12:58, Albert Astals Cid wrote: > El Dimecres, 14 de novembre de 2012, a les 07:28:21, Adam Reichold > va escriure: Hello Albert, > > Before I try the other classes, could you have a look at the > attached patch for SampledFunction and tell if this is what you are > looking for? > > Would something like memcpy(sampleSize, func->sampleSize, > funcMaxInputs * sizeof(int)); be preferred to the plain for-loops? > >> Yes. it's what the other functions use, right? > > > Regards, Adam. > > P.S.: By the way, is there a reason to use the signature > Function(const Function*) for the "what would be a copy > constructor" instead of "Function(const Function&)"? > >> No clue, ask the guy that wrote them (hint, he's not around) > >> Cheers, Albert > > > On 13.11.2012 23:28, Albert Astals Cid wrote: >>>> El Dilluns, 27 d'agost de 2012, a les 22:31:13, Albert Astals >>>> Cid >>>> >>>> va escriure: >>>>> El Dilluns, 27 d'agost de 2012, a les 08:29:40, Thomas >>>>> Freitag va >>>>> >>>>> escriure: >>>>>> On 27.08.2012 00:56, Albert Astals Cid wrote: >>>>>>> El Diumenge, 26 d'agost de 2012, a les 15:48:37, He Liu >>>>>>> va >>>>>>> >>>>>>> escriure: >>>>>>>>>> 5. vtable pointer will be overwritten >>>>>>>>>> Function.cc:422:10: warning: destination for >>>>>>>>>> this 'memcpy' call is a pointer to dynamic class >>>>>>>>>> 'SampledFunction'; vtable pointer will be >>>>>>>>>> overwritten [-Wdynamic-class-memaccess] >>>>>>>>>> >>>>>>>>>> memcpy(this, func, sizeof(SampledFunction)); >>>>>>>>>> ~~~~~~ ^ >>>>>>>>>> >>>>>>>>>> Function.cc:422:10: note: explicitly cast the >>>>>>>>>> pointer to silence this warning >>>>>>>>>> >>>>>>>>>> At least categrory 5. sound serious to me, I >>>>>>>>>> would never have copied instances of C++ objects >>>>>>>>>> in that way, because it depends on the compiler >>>>>>>>>> and the class if this causes problems on runtime, >>>>>>>>>> s. i.e. >>>>>>>>>> http://weseetips.com/tag/afx_zero_init_object/, >>>>>>>>> >>>>>>>>> Note this is memset-ing to 0, not memcpy-ing a >>>>>>>>> class to itself. To be honest i agree memcpy'in a >>>>>>>>> SampledFunction to a SampledFunction is ugly, but i >>>>>>>>> fail to see why it would not work. >>>>>> >>>>>> It works, at least with the actual used compilers. But it >>>>>> works only, because the allocated members of >>>>>> SampledFunction are overwriten after doing the memcpy. >>>>>> And this behaviour makes it just more ugly in my eyes. >>>>>> And this is the same with the other memcpy's in >>>>>> ExponentialFunction, StitchingFunction and >>>>>> PostScriptFunction. And it will work till such time as >>>>>> everybody who changes the class will not forget to do it >>>>>> in the same way. So if You are not willing to change it >>>>>> (or let somebody else change it, I know, never change >>>>>> running code), we could use the hint: explicitly cast the >>>>>> pointer to silence this warning. >>>>> >>>>> I'm fine with someone fixing the code. Less warnings is >>>>> always good, but for master, there's no need to introduce a >>>>> pontential breaker just because clang complains. >>>> >>>> So at the end noone volunteered to re-rewite those functions >>>> not to use memcpy over classes with virtuals? >>>> >>>> Cheers, Albert >>>> >>>>> Cheers, Albert >>>>> >>>>>> Cheers, Thomas >>>>>> >>>>>>>> Hi Albert, >>>>>>>> >>>>>>>> :-) >>>>>>>> >>>>>>>> A pointer of type SampleFunction* could be pointing >>>>>>>> to an instance of a SampleFunction sub-class, which >>>>>>>> has different vtable contents. >>>>>>> >>>>>>> No, it could not, SampledFunction does not have any >>>>>>> childs and the function doing that memcopy is private >>>>>>> anyway. >>>>>>> >>>>>>>> As a result, one could construct a SampleFunction >>>>>>>> with SampleFunction(SampleFunction *) using a pointer >>>>>>>> to a sub-class instance, and overwrite the >>>>>>>> SampleFunction's vtable address with the sub-class's >>>>>>>> vtable address. >>>>>>>> >>>>>>>> I am not sure if it will lead to any >>>>>>>> bugs/vulnerabilities in this case, but it is not safe >>>>>>>> practice in general. >>>>>>> >>>>>>> Sure, i never said it was. I'm just saying i don't see >>>>>>> why it would not work in our case. >>>>>>> >>>>>>>> Since the vtable structure depends on how the >>>>>>>> compiler is implemented, memcpy or memset on object >>>>>>>> pointers will generally lead to undefined behaviors. >>>>>>> >>>>>>> I'm far from a compiler expert, but one would hope that >>>>>>> for a given class the compiler stores always the >>>>>>> "stuff" in the same order in memory, so again, i fail >>>>>>> to see why this should fail in our case. >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> Albert >>>>>>> >>>>>>>> Thanks. >>>>>>>> _______________________________________________ >>>>>>>> poppler mailing list [email protected] >>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>>>>> >>>>>>> >>>>>>>> _______________________________________________ poppler >>>>>>> mailing list [email protected] >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>>>>> >>>>>>> . >>>>>> >>>>>> _______________________________________________ poppler >>>>>> mailing list [email protected] >>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>>> >>>>> _______________________________________________ poppler >>>>> mailing list [email protected] >>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>> >>>> _______________________________________________ poppler >>>> mailing list [email protected] >>>> http://lists.freedesktop.org/mailman/listinfo/poppler > > _______________________________________________ poppler mailing > list [email protected] > http://lists.freedesktop.org/mailman/listinfo/poppler > > -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQEcBAEBAgAGBQJQo9lqAAoJEPSSjE3STU34xZEIAL34RZqQK3gnFvZ7LVYLM6rf lpSG1qn0P0G5h1qexVtx9gMxoaN4m2lKMS2N9eomNu3IwxgP5D7phwdSfJesCkDS gd19EQ+oDtwKdeLdQMq+oM63mYzr6nyVUHlPSFtIO4Z1VBdZCMsowQnTsUe45UJ9 HWzqzzwiX91OTh7ILZqpQ4UtgmjZRIZmGxJVOqJcAK+qdVh7HiYmCbjGKnZ+YVdp NtW2jWTTg6GL+rEMQS1WTR1oDgfIPLgQWAdULezS3fsOQQvthUMlQuZQMgeG5cVR QGbQIjKTLjw/TNo+oQ+5r/lLfHynFYp3VNuAv/LCIHMeGZANpfKaQeTaao2ecMk= =fieh -----END PGP SIGNATURE-----
>From 59f4f6d3c62603403043f18a66a54b2735f5f239 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Wed, 14 Nov 2012 18:22:41 +0100 Subject: [PATCH 1/5] add Function copy constructor and rework SampledFunction copy constructor --- poppler/Function.cc | 33 +++++++++++++++++++++++++++++++-- poppler/Function.h | 4 +++- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/poppler/Function.cc b/poppler/Function.cc index d26aed8..b288f73 100644 --- a/poppler/Function.cc +++ b/poppler/Function.cc @@ -178,6 +178,16 @@ GBool Function::init(Dict *dict) { return gFalse; } +Function::Function(const Function* func) { + m = func->m; + n = func->n; + + memcpy(domain, func->domain, funcMaxInputs * 2 * sizeof(double)); + memcpy(range, func->range, funcMaxOutputs * 2 * sizeof(double)); + + hasRange = func->hasRange; +} + //------------------------------------------------------------------------ // IdentityFunction //------------------------------------------------------------------------ @@ -419,13 +429,32 @@ SampledFunction::~SampledFunction() { } } -SampledFunction::SampledFunction(SampledFunction *func) { - memcpy(this, func, sizeof(SampledFunction)); +SampledFunction::SampledFunction(const SampledFunction *func) : Function(func) { + // parameters + + memcpy(sampleSize, func->sampleSize, funcMaxInputs * sizeof(int)); + + memcpy(encode, func->encode, funcMaxInputs * 2 * sizeof(double)); + memcpy(decode, func->decode, funcMaxInputs * 2 * sizeof(double)); + + memcpy(inputMul, func->inputMul, funcMaxInputs * sizeof(double)); + + ok = func->ok; + + // samples + idxOffset = (int *)gmallocn(1 << m, sizeof(int)); memcpy(idxOffset, func->idxOffset, (1 << m) * (int)sizeof(int)); + samples = (double *)gmallocn(nSamples, sizeof(double)); memcpy(samples, func->samples, nSamples * sizeof(double)); + sBuf = (double *)gmallocn(1 << m, sizeof(double)); + + // buffers + + memcpy(cacheIn, func->cacheIn, funcMaxInputs * sizeof(double)); + memcpy(cacheOut, func->cacheOut, funcMaxOutputs * sizeof(double)); } void SampledFunction::transform(double *in, double *out) { diff --git a/poppler/Function.h b/poppler/Function.h index 25df133..23ea3ab 100644 --- a/poppler/Function.h +++ b/poppler/Function.h @@ -90,6 +90,8 @@ public: protected: static Function *parse(Object *funcObj, std::set<int> *usedParents); + Function(const Function *func); + int m, n; // size of input and output tuples double // min and max values for function domain domain[funcMaxInputs][2]; @@ -140,7 +142,7 @@ public: private: - SampledFunction(SampledFunction *func); + SampledFunction(const SampledFunction *func); int // number of samples for each domain element sampleSize[funcMaxInputs]; -- 1.8.0 >From 4a015d90a92d7ea30886e580242f4658c9ac95b6 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Wed, 14 Nov 2012 18:30:18 +0100 Subject: [PATCH 2/5] rework ExponentialFunction copy constructor and fixed a mistake in SampledFunction copy constructor --- poppler/Function.cc | 12 +++++++++--- poppler/Function.h | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/poppler/Function.cc b/poppler/Function.cc index b288f73..28b8286 100644 --- a/poppler/Function.cc +++ b/poppler/Function.cc @@ -435,7 +435,7 @@ SampledFunction::SampledFunction(const SampledFunction *func) : Function(func) { memcpy(sampleSize, func->sampleSize, funcMaxInputs * sizeof(int)); memcpy(encode, func->encode, funcMaxInputs * 2 * sizeof(double)); - memcpy(decode, func->decode, funcMaxInputs * 2 * sizeof(double)); + memcpy(decode, func->decode, funcMaxOutputs * 2 * sizeof(double)); memcpy(inputMul, func->inputMul, funcMaxInputs * sizeof(double)); @@ -645,8 +645,14 @@ ExponentialFunction::ExponentialFunction(Object *funcObj, Dict *dict) { ExponentialFunction::~ExponentialFunction() { } -ExponentialFunction::ExponentialFunction(ExponentialFunction *func) { - memcpy(this, func, sizeof(ExponentialFunction)); +ExponentialFunction::ExponentialFunction(const ExponentialFunction *func) : Function(func) { + memcpy(c0, func->c0, funcMaxOutputs * sizeof(double)); + memcpy(c1, func->c1, funcMaxOutputs * sizeof(double)); + + e = func->e; + isLinear = func->isLinear; + + ok = func->ok; } void ExponentialFunction::transform(double *in, double *out) { diff --git a/poppler/Function.h b/poppler/Function.h index 23ea3ab..2a61a4c 100644 --- a/poppler/Function.h +++ b/poppler/Function.h @@ -181,7 +181,7 @@ public: private: - ExponentialFunction(ExponentialFunction *func); + ExponentialFunction(const ExponentialFunction *func); double c0[funcMaxOutputs]; double c1[funcMaxOutputs]; -- 1.8.0 >From 9fc274b30ff7f7e98788b7f72d0cdf318a434479 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Wed, 14 Nov 2012 18:35:01 +0100 Subject: [PATCH 3/5] reworked StitchedFunction copy constructor --- poppler/Function.cc | 16 +++++++++++----- poppler/Function.h | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/poppler/Function.cc b/poppler/Function.cc index 28b8286..c36f2a5 100644 --- a/poppler/Function.cc +++ b/poppler/Function.cc @@ -796,21 +796,27 @@ StitchingFunction::StitchingFunction(Object *funcObj, Dict *dict, std::set<int> obj1.free(); } -StitchingFunction::StitchingFunction(StitchingFunction *func) { - int i; +StitchingFunction::StitchingFunction(const StitchingFunction *func) : Function(func) { + k = func->k; + ok = func->ok; + + // functions - memcpy(this, func, sizeof(StitchingFunction)); funcs = (Function **)gmallocn(k, sizeof(Function *)); - for (i = 0; i < k; ++i) { + for (int i = 0; i < k; ++i) { funcs[i] = func->funcs[i]->copy(); } + + // parameters + bounds = (double *)gmallocn(k + 1, sizeof(double)); memcpy(bounds, func->bounds, (k + 1) * sizeof(double)); + encode = (double *)gmallocn(2 * k, sizeof(double)); memcpy(encode, func->encode, 2 * k * sizeof(double)); + scale = (double *)gmallocn(k, sizeof(double)); memcpy(scale, func->scale, k * sizeof(double)); - ok = gTrue; } StitchingFunction::~StitchingFunction() { diff --git a/poppler/Function.h b/poppler/Function.h index 2a61a4c..7a2dc1b 100644 --- a/poppler/Function.h +++ b/poppler/Function.h @@ -212,7 +212,7 @@ public: private: - StitchingFunction(StitchingFunction *func); + StitchingFunction(const StitchingFunction *func); int k; Function **funcs; -- 1.8.0 >From 6ccf4dfb03ff93a14a4424012f228e4d39e274a5 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Wed, 14 Nov 2012 18:43:13 +0100 Subject: [PATCH 4/5] reworked PostScriptFunction copy constructor --- poppler/Function.cc | 14 ++++++++++++-- poppler/Function.h | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/poppler/Function.cc b/poppler/Function.cc index c36f2a5..4fe6282 100644 --- a/poppler/Function.cc +++ b/poppler/Function.cc @@ -1225,11 +1225,21 @@ PostScriptFunction::PostScriptFunction(Object *funcObj, Dict *dict) { return; } -PostScriptFunction::PostScriptFunction(PostScriptFunction *func) { - memcpy(this, func, sizeof(PostScriptFunction)); +PostScriptFunction::PostScriptFunction(const PostScriptFunction *func) : Function(func) { + codeSize = func->codeSize; + ok = func->ok; + + // code + code = (PSObject *)gmallocn(codeSize, sizeof(PSObject)); memcpy(code, func->code, codeSize * sizeof(PSObject)); + codeString = func->codeString->copy(); + + // buffers + + memcpy(cacheIn, func->cacheIn, funcMaxInputs * sizeof(double)); + memcpy(cacheOut, func->cacheOut, funcMaxOutputs * sizeof(double)); } PostScriptFunction::~PostScriptFunction() { diff --git a/poppler/Function.h b/poppler/Function.h index 7a2dc1b..b361dd4 100644 --- a/poppler/Function.h +++ b/poppler/Function.h @@ -240,7 +240,7 @@ public: private: - PostScriptFunction(PostScriptFunction *func); + PostScriptFunction(const PostScriptFunction *func); GBool parseCode(Stream *str, int *codePtr); GooString *getToken(Stream *str); void resizeCode(int newSize); -- 1.8.0 >From 92d7c4f87118222e383189cf5ed3e9aba57ef2d1 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Wed, 14 Nov 2012 18:46:37 +0100 Subject: [PATCH 5/5] mark Function::copy and its overrides const as a precaution --- poppler/Function.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/poppler/Function.h b/poppler/Function.h index b361dd4..1ff5973 100644 --- a/poppler/Function.h +++ b/poppler/Function.h @@ -61,7 +61,7 @@ public: // Initialize the entries common to all function types. GBool init(Dict *dict); - virtual Function *copy() = 0; + virtual Function *copy() const = 0; // Return the function type: // -1 : identity @@ -109,7 +109,7 @@ public: IdentityFunction(); virtual ~IdentityFunction(); - virtual Function *copy() { return new IdentityFunction(); } + virtual Function *copy() const { return new IdentityFunction(); } virtual int getType() { return -1; } virtual void transform(double *in, double *out); virtual GBool isOk() { return gTrue; } @@ -126,7 +126,7 @@ public: SampledFunction(Object *funcObj, Dict *dict); virtual ~SampledFunction(); - virtual Function *copy() { return new SampledFunction(this); } + virtual Function *copy() const { return new SampledFunction(this); } virtual int getType() { return 0; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } @@ -170,7 +170,7 @@ public: ExponentialFunction(Object *funcObj, Dict *dict); virtual ~ExponentialFunction(); - virtual Function *copy() { return new ExponentialFunction(this); } + virtual Function *copy() const { return new ExponentialFunction(this); } virtual int getType() { return 2; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } @@ -199,7 +199,7 @@ public: StitchingFunction(Object *funcObj, Dict *dict, std::set<int> *usedParents); virtual ~StitchingFunction(); - virtual Function *copy() { return new StitchingFunction(this); } + virtual Function *copy() const { return new StitchingFunction(this); } virtual int getType() { return 3; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } @@ -231,7 +231,7 @@ public: PostScriptFunction(Object *funcObj, Dict *dict); virtual ~PostScriptFunction(); - virtual Function *copy() { return new PostScriptFunction(this); } + virtual Function *copy() const { return new PostScriptFunction(this); } virtual int getType() { return 4; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } -- 1.8.0
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
