From dab881139d1fa26683fce51676e1f59b45deb02f Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 22 Nov 2021 15:16:00 -0500 Subject: [PATCH] Pad: Fix `copyPadWithoutHistory` apool corruption bug --- CHANGELOG.md | 4 +- src/node/db/Pad.js | 3 +- src/static/js/AttributePool.js | 21 ++++++++- src/tests/backend/specs/api/pad.js | 73 ++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e1f3f565..e11c6b691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # 1.9.0 (not yet released) -### Notable enhancements +### Notable enhancements and fixes + +* Fixed a potential attribute pool corruption bug with `copyPadWithoutHistory`. #### For plugin authors diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index d412037df..2aed5fd23 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -492,10 +492,9 @@ Pad.prototype.copyPadWithoutHistory = async function (destinationID, force) { // initialize the pad with a new line to avoid getting the defaultText const newPad = await padManager.getPad(destinationID, '\n'); + newPad.pool = sourcePad.pool.clone(); const oldAText = this.atext; - const newPool = newPad.pool; - newPool.fromJsonable(sourcePad.pool.toJsonable()); // copy that sourceId pool to the new pad // based on Changeset.makeSplice const assem = Changeset.smartOpAssembler(); diff --git a/src/static/js/AttributePool.js b/src/static/js/AttributePool.js index d8e587654..d46b207bb 100644 --- a/src/static/js/AttributePool.js +++ b/src/static/js/AttributePool.js @@ -91,6 +91,17 @@ class AttributePool { this.nextNum = 0; } + /** + * @returns {AttributePool} A deep copy of this attribute pool. + */ + clone() { + const c = new AttributePool(); + for (const [n, a] of Object.entries(this.numToAttrib)) c.numToAttrib[n] = [a[0], a[1]]; + Object.assign(c.attribToNum, this.attribToNum); + c.nextNum = this.nextNum; + return c; + } + /** * Add an attribute to the attribute set, or query for an existing attribute identifier. * @@ -164,7 +175,10 @@ class AttributePool { /** * @returns {Jsonable} An object that can be passed to `fromJsonable` to reconstruct this - * attribute pool. The returned object can be converted to JSON. + * attribute pool. The returned object can be converted to JSON. WARNING: The returned object + * has references to internal state (it is not a deep copy). Use the `clone()` method to copy + * a pool -- do NOT do `new AttributePool().fromJsonable(pool.toJsonable())` to copy because + * the resulting shared state will lead to pool corruption. */ toJsonable() { return { @@ -177,7 +191,10 @@ class AttributePool { * Replace the contents of this attribute pool with values from a previous call to `toJsonable`. * * @param {Jsonable} obj - Object returned by `toJsonable` containing the attributes and their - * identifiers. + * identifiers. WARNING: This function takes ownership of the object (it does not make a deep + * copy). Use the `clone()` method to copy a pool -- do NOT do + * `new AttributePool().fromJsonable(pool.toJsonable())` to copy because the resulting shared + * state will lead to pool corruption. */ fromJsonable(obj) { this.numToAttrib = obj.numToAttrib; diff --git a/src/tests/backend/specs/api/pad.js b/src/tests/backend/specs/api/pad.js index ab56c8ca4..7aab137b9 100644 --- a/src/tests/backend/specs/api/pad.js +++ b/src/tests/backend/specs/api/pad.js @@ -9,6 +9,7 @@ const assert = require('assert').strict; const common = require('../../common'); +const padManager = require('../../../../node/db/PadManager'); let agent; const apiKey = common.apiKey; @@ -547,6 +548,78 @@ describe(__filename, function () { assert.equal(res.body.code, 0); }); }); + + // Regression test for https://github.com/ether/etherpad-lite/issues/5296 + it('source and destination attribute pools are independent', async function () { + // Strategy for this test: + // 1. Create a new pad without bold or italic text + // 2. Use copyPadWithoutHistory to copy the pad. + // 3. Add some bold text (but no italic text!) to the source pad. This should add a bold + // attribute to the source pad's pool but not to the destination pad's pool. + // 4. Add some italic text (but no bold text!) to the destination pad. This should add an + // italic attribute to the destination pad's pool with the same number as the newly added + // bold attribute in the source pad's pool. + // 5. Add some more text (bold or plain) to the source pad. This will save the source pad to + // the database after the destination pad has had an opportunity to corrupt the source + // pad. + // 6. Export the source and destination pads. Make sure that doesn't appear in the + // source pad's HTML, and that doesn't appear int he destination pad's HTML. + // 7. Force the server to re-init the pads from the database. + // 8. Repeat step 6. + // If appears in the source pad, or appears in the destination pad, then shared + // state between the two attribute pools caused corruption. + + const getHtml = async (padId) => { + const res = await agent.get(`${endPoint('getHTML')}&padID=${padId}`) + .expect(200) + .expect('Content-Type', /json/); + assert.equal(res.body.code, 0); + return res.body.data.html; + }; + + const setBody = async (padId, bodyHtml) => { + await agent.post(endPoint('setHTML')) + .send({padID: padId, html: `${bodyHtml}`}) + .expect(200) + .expect('Content-Type', /json/) + .expect((res) => assert.equal(res.body.code, 0)); + }; + + const origHtml = await getHtml(sourcePadId); + assert.doesNotMatch(origHtml, //); + assert.doesNotMatch(origHtml, //); + await agent.get(`${endPoint('copyPadWithoutHistory')}&sourceID=${sourcePadId}` + + `&destinationID=${newPad}&force=false`) + .expect(200) + .expect('Content-Type', /json/) + .expect((res) => assert.equal(res.body.code, 0)); + + const newBodySrc = 'bold'; + const newBodyDst = 'italic'; + await setBody(sourcePadId, newBodySrc); + await setBody(newPad, newBodyDst); + await setBody(sourcePadId, `${newBodySrc} foo`); + + let [srcHtml, dstHtml] = await Promise.all([getHtml(sourcePadId), getHtml(newPad)]); + assert.match(srcHtml, new RegExp(newBodySrc)); + assert.match(dstHtml, new RegExp(newBodyDst)); + + // Force the server to re-read the pads from the database. This rebuilds the attribute pool + // objects from scratch, ensuring that an internally inconsistent attribute pool object did + // not cause the above tests to accidentally pass. + const reInitPad = async (padId) => { + const pad = await padManager.getPad(padId); + await pad.init(); + }; + await Promise.all([ + reInitPad(sourcePadId), + reInitPad(newPad), + ]); + + [srcHtml, dstHtml] = await Promise.all([getHtml(sourcePadId), getHtml(newPad)]); + assert.match(srcHtml, new RegExp(newBodySrc)); + assert.match(dstHtml, new RegExp(newBodyDst)); + }); }); });