Closed Bug 1352572 Opened 7 years ago Closed 7 years ago

Remove NPN_PostURL with file contents

Categories

(Core Graveyard :: Plug-ins, enhancement, P2)

Product:
Core Graveyard
Component:
Plug-ins
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: benjamin, Assigned: lie.1296, Mentored)

References

Details

Attachments

(8 files, 4 obsolete files)

Bug 1352572 - Fix buffer overflow in verbose debug log;
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352572 - Add support in nptest plugin to test NPN_PostURL(file=true);
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352572 - Return an NPERR_GENERIC_ERROR if plugin tries to call NPN_PostURL(file=true);
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352572 - Fix misleading/incorrect comment in streamTest;
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352572 - Implement nptest.cpp:streamTest(postFile) parameter;
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352572 - Add negative test for NPN_PostURLNotify();
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352572 - Return NPERR_GENERIC_ERROR if plugin calls NPN_PostURLNotify(file=true);
59 bytes, text/x-review-board-request
benjamin
: review+
Details
Bug 1352572 - Remove unused code that implement NPN_PostURLNotify(file=true) and NPN_PostURL(file=true);
59 bytes, text/x-review-board-request
benjamin
: review+
Details 
NPN_PostURL and NPN_PostURLNotify have two ways to specify the data to post: it can either be served as a buffer or as a file path. the `file` parameter specifies which the plugin wants to use.

We want to remove support for the file version of this API and only support the buffer version. Flash only uses the buffer version, and sandboxing means the file version is quite complicated.

This is primarily implemented in nsPluginHost::PostURL, but we can reject the file==true case much earlier up the stack in _posturl or _posturlnotify and not pass that through MakeNewNPAPIStreamInternal at all. If the plugin does pass file==true we should early return with NPERR_GENERIC_ERROR.

We can also do the check in the plugin process at _posturl _posturlnotify in PluginModuleChild.cpp for early sanity-checking, although that doesn't matter too much.
I don't know whether there is any testing code for this file version or not! If there is it will show up while running the mochitests in dom/plugin and with testing code in nptest.cpp
Mentor: benjamin
Priority: -- → P2
A couple questions:

1. Does the bug expect anyone still trying to call the unsupported functions to receive an error message (on dev-console?) or should they simply be removed?
2. Is a negative test (test trying to call the removed functionality) needed?
Flags: needinfo?(benjamin)
The function should return NPERR_GENERIC_ERROR in this case, but we don't need anything else in the error console. Adding a PLUGIN_LOG_DEBUG would be nice.

It would be nice but not necessary to add a negative test to make sure we return the correct error code (and don't crash).
Flags: needinfo?(benjamin)
Assignee: nobody → lie.1296
Status: NEW → ASSIGNED
Comment on attachment 8857542 [details]
Bug 1352572 - Fix buffer overflow in verbose debug log;

https://reviewboard.mozilla.org/r/129512/#review134080

Good catch!
Attachment #8857542 - Flags: review?(benjamin) → review+
Comment on attachment 8857543 [details]
Bug 1352572 - Add support in nptest plugin to test NPN_PostURL(file=true);

https://reviewboard.mozilla.org/r/129514/#review134086

::: dom/plugins/test/testplugin/README:350
(Diff revision 1)
>  "posturl": a url.  After the plugin receives a stream, and NPP_DestroyStream
>    is called, if "posturl" is specified, the plugin will post the contents
>    of the stream to the specified url via NPN_PostURL.  See "postmode" for
>    additional details.
>  
> -"postmode": either "frame" or "stream".  If "frame", and a "frame" attribute
> +"postmode": either "frame", "stream", "frame-file", or "stream-file".  If

I don't think this is the right layer to accomplish the goal here. I think you could accomplish the same general goal of testing the file variants of NPP_Post by adding another argument to the "streamTest" method, or even writing a new testing method.
Attachment #8857543 - Flags: review?(benjamin) → review-
Comment on attachment 8857544 [details]
Bug 1352572 - Refactor getEmbed() in pluginstream.js;

https://reviewboard.mozilla.org/r/129516/#review134088
Attachment #8857544 - Flags: review?(benjamin) → review+
Comment on attachment 8857545 [details]
Bug 1352572 - Add generic callback handling mechanism to testplugin;

https://reviewboard.mozilla.org/r/129518/#review134090

This is rather ingenious. I'm not sure it's necessary, but I'm going to mark r+ because it looks correct, and as I read the rest of the patch series I'll figure out whether this is actually worth taking in terms of extra code.
Attachment #8857545 - Flags: review?(benjamin) → review+
Comment on attachment 8857546 [details]
Bug 1352572 - Return an NPERR_GENERIC_ERROR if plugin tries to call NPN_PostURL(file=true);

https://reviewboard.mozilla.org/r/129520/#review134092

r- because of the test changes, the actual code change is fine.

::: dom/plugins/test/mochitest/pluginstream.js:32
(Diff revision 1)
>  
> -    is(embed.getError(), "pass", "plugin reported error");
> +    checkPluginError("pass", finishWhenCalled);
> +  }
> +
> +  function checkPluginError(expectedError = "pass", finishWhenCalled = true) {
> +    var embed = embed || getEmbed();

This || block doesn't do what you want. `var embed` is the first line of the block and so embed is always undefined.

::: dom/plugins/test/mochitest/test_pluginstream_postfile.html:29
(Diff revision 1)
> +   - if file is passed as true (i.e. does not crash).
> +   -->
> +  <embed src="loremipsum.txt" streammode="asfileonly"
> +         frame="testframe" posturl="post.sjs" postmode="frame-file"
> +         id="embedtest" style="width: 400px; height: 100px;"
> +         type="application/x-test" onposturlcomplete="checkPluginError('Error: NPN_PostURL returned error value 1')"></embed>

ok yes, based on prior commits I think it's better/sufficient to test this based on the .streamTest method, which probably means you don't need the onposturlcomplete callback either.

(ditto below)
Attachment #8857546 - Flags: review?(benjamin) → review-
Comment on attachment 8857547 [details]
Bug 1352572 - Fix misleading/incorrect comment in streamTest;

https://reviewboard.mozilla.org/r/129522/#review134094

Are the matching README docs correct?
Attachment #8857547 - Flags: review?(benjamin) → review+
Comment on attachment 8857548 [details]
Bug 1352572 - Implement nptest.cpp:streamTest(postFile) parameter;

https://reviewboard.mozilla.org/r/129524/#review134096

::: commit-message-224b1:4
(Diff revision 1)
> +Bug 1352572 - Implement nptest.cpp:streamTest(postFile) parameter; r?bsmedberg
> +
> +This is an optional parameter that can be used to test streams via file. It may
> +be necessary to back out this change hwen NPAPI sandbox is completed as the

nit misspelling

But realistically sandboxing doesn't matter, because we don't ever expect the plugin host to access the file, right? Just fail with your error code.

::: dom/plugins/test/testplugin/nptest.cpp:2747
(Diff revision 1)
>  }
>  
>  static bool
>  streamTest(NPObject* npobj, const NPVariant* args, uint32_t argCount, NPVariant* result)
>  {
> -  // .streamTest(url, doPost, postData, writeCallback, notifyCallback, redirectCallback, allowRedirects)
> +  // .streamTest(url, doPost, postData, writeCallback, notifyCallback, redirectCallback, allowRedirects, postFile = false)

Please update the nptest README with this information also.
Attachment #8857548 - Flags: review?(benjamin) → review-
Comment on attachment 8857549 [details]
Bug 1352572 - Add negative test for NPN_PostURLNotify();

https://reviewboard.mozilla.org/r/129526/#review134098

::: dom/plugins/test/testplugin/nptest.cpp:2815
(Diff revision 1)
>      }
>      postFile = NPVARIANT_TO_BOOLEAN(args[7]);
>    }
>  
> +  string buf;
> +  if (postFile) {

Oh, if the test plugin is going to write this file, this change should have been in the previous patch.

As is, I wouldn't want the test plugin to do this (for the sandboxing reasons you mention). Instead let's have the test itself write the file (using SpecialPowers?).

Otherwise this is too much complexity for something which we know is going to be false.
Attachment #8857549 - Flags: review?(benjamin) → review-
Comment on attachment 8857550 [details]
Bug 1352572 - Return NPERR_GENERIC_ERROR if plugin calls NPN_PostURLNotify(file=true);

https://reviewboard.mozilla.org/r/129528/#review134100
Attachment #8857550 - Flags: review?(benjamin) → review+
Comment on attachment 8857551 [details]
Bug 1352572 - Remove unused code that implement NPN_PostURLNotify(file=true) and NPN_PostURL(file=true);

https://reviewboard.mozilla.org/r/129530/#review134102

I feel like this should probably be merged with the next patch or two instead of landing separately, but I'll say more when I read the next few.
Attachment #8857551 - Flags: review?(benjamin) → review+
Comment on attachment 8857552 [details]
Bug 1352572 - Remove nsPluginHost::PostURL(isFile) parameter;

https://reviewboard.mozilla.org/r/129532/#review134104
Attachment #8857552 - Flags: review?(benjamin) → review+
Comment on attachment 8857553 [details]
Bug 1352572 - Remove nsPluginHost::CreateTempFileToPost();

https://reviewboard.mozilla.org/r/129534/#review134106
Attachment #8857553 - Flags: review?(benjamin) → review+
Comment on attachment 8857551 [details]
Bug 1352572 - Remove unused code that implement NPN_PostURLNotify(file=true) and NPN_PostURL(file=true);

https://reviewboard.mozilla.org/r/129530/#review134108

::: dom/plugins/base/nsNPAPIPlugin.cpp:436
(Diff revision 1)
>        break;
>      }
>    case eNPPStreamTypeInternal_Post:
>      {
> +      NPBool file = false;
>        if (NS_FAILED(pluginHost->PostURL(inst, relativeURL, len, buf, file,

Yes please merge these last three commits, so that this extra `file` variable goes away in one swoop.
Comment on attachment 8857547 [details]
Bug 1352572 - Fix misleading/incorrect comment in streamTest;

https://reviewboard.mozilla.org/r/129522/#review134094

The streamTest documentation in the README already have the correct parameter name for this
Attachment #8857552 - Attachment is obsolete: true
Attachment #8857553 - Attachment is obsolete: true
Attachment #8857544 - Attachment is obsolete: true
Attachment #8857545 - Attachment is obsolete: true
Comment on attachment 8857543 [details]
Bug 1352572 - Add support in nptest plugin to test NPN_PostURL(file=true);

https://reviewboard.mozilla.org/r/129514/#review138094
Attachment #8857543 - Flags: review?(benjamin) → review+
Comment on attachment 8857546 [details]
Bug 1352572 - Return an NPERR_GENERIC_ERROR if plugin tries to call NPN_PostURL(file=true);

https://reviewboard.mozilla.org/r/129520/#review138096
Attachment #8857546 - Flags: review?(benjamin) → review+
Comment on attachment 8857548 [details]
Bug 1352572 - Implement nptest.cpp:streamTest(postFile) parameter;

https://reviewboard.mozilla.org/r/129524/#review138098
Attachment #8857548 - Flags: review?(benjamin) → review+
Comment on attachment 8857549 [details]
Bug 1352572 - Add negative test for NPN_PostURLNotify();

https://reviewboard.mozilla.org/r/129526/#review138100
Attachment #8857549 - Flags: review?(benjamin) → review+
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/721ffdb73f9e
Fix buffer overflow in verbose debug log; r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/389ba434b233
Add support in nptest plugin to test NPN_PostURL(file=true); r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/e78a21fb4959
Return an NPERR_GENERIC_ERROR if plugin tries to call NPN_PostURL(file=true); r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/b0833df319cc
Fix misleading/incorrect comment in streamTest; r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/97b04324897e
Implement nptest.cpp:streamTest(postFile) parameter; r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/ab66ad2abaa0
Add negative test for NPN_PostURLNotify(); r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/102c0159724c
Return NPERR_GENERIC_ERROR if plugin calls NPN_PostURLNotify(file=true); r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/165b7df7dc80
Remove unused code that implement NPN_PostURLNotify(file=true) and NPN_PostURL(file=true); r=bsmedberg
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: