New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 112983
Owner:
Last visit > 30 days ago
Closed: Feb 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug-Security

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
link

Issue 106742: net::URLRequestFtpJob::StartTransaction() - crash

Reported by slaw...@gmail.com, Dec 7 2011

Issue description

Crashes on windows dev 17.0.963.0 (113143) and canary 17.0.963.1 (113335).
It looks like null ptr but used security template for safety - it crashes in browser process.

Repro:
----- crash1.html -----
<audio src="ftp://foo.bar/">
-----------------------

(1284.14b8): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=02d776c0 ebx=0483c8c0 ecx=00000000 edx=00000002 esi=047f9c60 edi=047f9c60
eip=579ddee7 esp=0304efd4 ebp=0304eff0 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010246
chrome_57120000!net::URLRequestFtpJob::StartTransaction+0x1b:
579ddee7 8b01            mov     eax,dword ptr [ecx]  ds:0023:00000000=????????

ExceptionAddress: 579ddee7 (chrome_57120000!net::URLRequestFtpJob::StartTransaction+0x0000001b)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: 00000000
Attempt to read from address 00000000

ChildEBP RetAddr  
0304eff0 5795a8f3 chrome_57120000!net::URLRequestFtpJob::StartTransaction+0x1b
0304f008 5795a69b chrome_57120000!net::URLRequest::StartJob+0xe4
0304f020 57788365 chrome_57120000!net::URLRequest::Start+0x79
0304f070 5774ab48 chrome_57120000!ResourceQueue::AddRequest+0xa4
0304f0a0 5774ab27 chrome_57120000!ResourceDispatcherHost::InsertIntoResourceQueue+0x1a
0304f0f0 57748a46 chrome_57120000!ResourceDispatcherHost::BeginRequestInternal+0x1ec
0304f2e0 577482dc chrome_57120000!ResourceDispatcherHost::BeginRequest+0x74c
0304f2f4 5774e9fd chrome_57120000!ResourceDispatcherHost::OnRequestResource+0x19
0304f4c8 57747ff9 chrome_57120000!ResourceHostMsg_RequestResource::Dispatch<ResourceDispatcherHost,ResourceDispatcherHost,int,ResourceHostMsg_Request const &>+0x53
0304f500 57795f51 chrome_57120000!ResourceDispatcherHost::OnMessageReceived+0x147
0304f510 577471b0 chrome_57120000!ResourceMessageFilter::OnMessageReceived+0x13
0304f534 577470dc chrome_57120000!BrowserMessageFilter::DispatchMessageW+0x20
0304f570 57904886 chrome_57120000!BrowserMessageFilter::OnMessageReceived+0x33
0304f580 579048b1 chrome_57120000!IPC::ChannelProxy::Context::TryFilters+0x26
0304f58c 5712f30e chrome_57120000!IPC::ChannelProxy::Context::OnMessageReceived+0xe
0304f6fc 5712f5c8 chrome_57120000!IPC::Channel::ChannelImpl::ProcessIncomingMessages+0x15b
0304f71c 571e08e9 chrome_57120000!IPC::Channel::ChannelImpl::OnIOCompleted+0x5a
0304f750 571e07e1 chrome_57120000!base::MessagePumpForIO::WaitForIOCompletion+0x9f
0304f76c 571e02ae chrome_57120000!base::MessagePumpForIO::DoRunLoop+0x17
0304f788 571e016f chrome_57120000!base::MessagePumpWin::RunWithDispatcher+0x38
0304f794 571c721b chrome_57120000!base::MessagePumpWin::Run+0xe
0304f7a0 571c71a0 chrome_57120000!MessageLoop::RunInternal+0x31
[...]
 
stack1.txt
4.1 KB View Download
crash1.html
51 bytes View Download

Comment 1 by skylined@chromium.org, Dec 7 2011

Labels: -Pri-0 -Area-Undefined Pri-3 Area-Internals OS-All SecSeverity-None
Owner: skylined@chromium.org
Status: Assigned
There's a NULL ftp_transaction_factory ptr; a debug build hits two DCHECKs before it hits a NULL ptr. Only affects latest trunk, so change must have been introduced recently.

src\net\url_request\url_request_ftp_job.cc @ line 34
URLRequestJob* URLRequestFtpJob::Factory(URLRequest* request,
                                         const std::string& scheme) {
  DCHECK_EQ(scheme, "ftp");

  int port = request->url().IntPort();
  if (request->url().has_port() &&
    !IsPortAllowedByFtp(port) && !IsPortAllowedByOverride(port))
    return new URLRequestErrorJob(request, ERR_UNSAFE_PORT);

  DCHECK(request->context());
  DCHECK(request->context()->ftp_transaction_factory()); ******** DCHECK ********
  return new URLRequestFtpJob(request);
}

src\net\url_request\url_request_ftp_job.cc @ line 66
void URLRequestFtpJob::StartTransaction() {
  // Create a transaction.
  DCHECK(!transaction_.get());
  DCHECK(request_->context());
  DCHECK(request_->context()->ftp_transaction_factory()); ******** DCHECK ********

  transaction_.reset(
      request_->context()->ftp_transaction_factory()->CreateTransaction()); ******** NULL PTR ********

  // No matter what, we want to report our status as IO pending since we will
  // be notifying our consumer asynchronously via OnStartCompleted.
  SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
  int rv;
  if (transaction_.get()) {
    rv = transaction_->Start(
        &request_info_, &start_callback_, request_->net_log());
    if (rv == ERR_IO_PENDING)
      return;
  } else {
    rv = ERR_FAILED;
  }
  // The transaction started synchronously, but we need to notify the
  // URLRequest delegate via the message loop.
  MessageLoop::current()->PostTask(
      FROM_HERE,
      method_factory_.NewRunnableMethod(
          &URLRequestFtpJob::OnStartCompleted, rv));
}

Comment 2 by scarybea...@gmail.com, Dec 8 2011

Labels: -SecSeverity-None SecSeverity-Low SecImpacts-None
@skylined: we've typically rated a simple, reliable crash of the browser process as SecSeverity-Low.
Also, don't forget the SecImpacts labels :) In this case, a trunk-only regression qualifies as SecImpacts-None

Comment 3 by scarybea...@gmail.com, Dec 8 2011

And Mstone needs to be set to 17 or 18, depending on whether the regression was introduced before or after the M17 branch point. Can you take care of it?

Comment 4 by skylined@chromium.org, Dec 8 2011

Will do - I didn't get around to doing that yesterday, so I assigned it to me for follow-up today.

Comment 5 by skylined@chromium.org, Dec 8 2011

Labels: Mstone-17
Affected: 17+ (dev, canary trunk)
Not affected: 15 (stable) & 16 (beta)

Comment 6 by jsc...@chromium.org, Dec 13 2011

Labels: -SecImpacts-None SecImpacts-Beta
Bulk edit for pending m17 beta release.

Comment 7 by k...@google.com, Dec 19 2011

Labels: -Mstone-17 Mstone-18 MovedFrom-17
Moving bugs marked as Available but not blockers from M17 to M18.  Please move back if you think this is a blocker, and add the ReleaseBlock-Stable label.  If you're able.

Comment 8 by kenrb@chromium.org, Jan 6 2012

skylined have you made any progress on this one?

Comment 9 by skylined@chromium.org, Jan 9 2012

Cc: joi@chromium.org
No, I have no clue what's causing this. ClusterFuzzz can't reproduce.

This chaneg may have something to do with it, joi@ can you have a look?
http://codereview.chromium.org/8769013

Comment 10 by joi@chromium.org, Jan 9 2012

According to the first comment on the bug, this was crashing as early as 113143 which is before my change that you linked to (it was committed as r113377).  Was there a special reason you thought r113377 was related?

Comment 11 by skylined@chromium.org, Jan 9 2012

Nope, I made some assumptions base on svn blame for chrome_url_request_context.cc and the stack at the time of the crash. I'm not familiar with the code - would you know how might help me find out what the problem is here?

Comment 12 by joi@chromium.org, Jan 9 2012

This seems like it would be in the net/ code so would look for possibly-related changes around the time the crash got introduced, and perhaps ask one of the net/ folks like wtc@ or eroman@ to take a quick peek.

Comment 13 by skylined@chromium.org, Jan 9 2012

Cc: -joi@chromium.org eroman@chromium.org
Thanks joi, eroman@: can you help me fix this?

Comment 14 by scarybea...@gmail.com, Feb 7 2012

@skylined: if you're not actively debugging and pushing this forward, could you find a new owner?

Comment 15 by skylined@chromium.org, Feb 7 2012

Sure, I'll try to get ClusterFuzz to help me pin down the change that introduced this:
https://cluster-fuzz.appspot.com/testcase?key=19176760

Comment 16 by kareng@google.com, Feb 7 2012

Labels: MovedFrom18 Mstone-19

Comment 17 by scarybea...@gmail.com, Feb 22 2012

@skylined: this seems to have gone two weeks without update. How is the fix progressing?

Comment 18 by skylined@chromium.org, Feb 23 2012

Cc: wtc@chromium.org
ClusterFuzz can't reproduce.

@wtc/eroman: can you help me fix this or find an owner?

@scarybeasts: Not.

Comment 19 by slaw...@gmail.com, Feb 23 2012

I can't reproduce it too. Tested on dev 19.0.1041.0 (121843) and canary 19.0.1049.1 (123065).

Comment 20 by scarybea...@gmail.com, Feb 23 2012

@slaweck: it's possible this was some transient regression that got taken care of shortly after it was introduced. If you're satisfied that recent builds seem ok, and we can close this out.

Comment 21 by slaw...@gmail.com, Feb 23 2012

Last time I saw it in dev 18.0.1025.7 (120697).
Yes, I think You can close this out.

Comment 22 by scarybea...@gmail.com, Feb 23 2012

Status: WontFix
Heisenbug! Bah.

Comment 23 by eroman@chromium.org, Feb 23 2012

Mergedinto: 112983
Status: Duplicate
This bug has already been fixed! See  issue 112983 

Comment 24 by bugdroid1@chromium.org, Oct 13 2012

Project Member
Labels: Restrict-AddIssueComment-Commit
Mergedinto: chromium:112983
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.

Comment 25 by bugdroid1@chromium.org, Mar 10 2013

Project Member
Labels: -Type-Security -Area-Internals -SecSeverity-Low -SecImpacts-Beta -Mstone-19 M-19 Security-Severity-Low Security-Impact-Beta Cr-Internals Type-Bug-Security

Comment 26 by bugdroid1@chromium.org, Mar 13 2013

Project Member
Labels: Restrict-View-EditIssue

Comment 27 by bugdroid1@chromium.org, Mar 14 2013

Project Member
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Comment 28 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Severity-Low Security_Severity-Low

Comment 29 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Impact-Beta Security_Impact-Beta

Comment 30 by ClusterFuzz, Feb 6 2014

Project Member
Labels: -Restrict-View-SecurityTeam -Restrict-View-EditIssue
Bulk update: removing view restriction from closed bugs.

Comment 31 by sheriffbot@chromium.org, Oct 1 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 32 by sheriffbot@chromium.org, Oct 2 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 33 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Sign in to add a comment