Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Crash in h2_progress_egress due to NULL stream #11379

Closed
jbgoog opened this issue Jun 23, 2023 · 7 comments
Closed

Crash in h2_progress_egress due to NULL stream #11379

jbgoog opened this issue Jun 23, 2023 · 7 comments
Assignees
@icing
Labels
crash HTTP/2

Comments

@jbgoog
Copy link

jbgoog commented Jun 23, 2023

I did this

Seeing a crash here during h2_progress_egress call:

    rv = nghttp2_submit_priority(ctx->h2, NGHTTP2_FLAG_NONE,
                                 stream->id, &pri_spec);

stream is NULL so stream->id throws the following error:

EXC_BAD_ACCESS / KERN_INVALID_ADDRESS @0x00000000

I expected the following

  • Application does not crash

curl/libcurl version

  • 8.1.2

operating system

iOS
@dfandrich
Copy link
Contributor

dfandrich commented Jun 23, 2023 via email

@jbgoog
Copy link
Author

jbgoog commented Jun 23, 2023

We include libcurl as a dependency of our iOS app, so there is no commandline for curl -V unfortunately.

It looks like this line to get the stream can be NULL:

struct stream_ctx *stream = H2_STREAM_CTX(data);

#define H2_STREAM_CTX(d)    ((struct stream_ctx *)(((d) && (d)->req.p.http)? \
                             ((struct HTTP *)(d)->req.p.http)->h2_ctx \
                               : NULL))

From https://github.com/curl/curl/blob/curl-8_1_2/lib/http2.c#L1659

When that happens, stream->id aborts.

It looks like stream here is only used to get the stream->id

    DEBUGF(LOG_CF(data, cf, "[h2sid=%d] Queuing PRIORITY",
                  stream->id));
    DEBUGASSERT(stream->id != -1);
    rv = nghttp2_submit_priority(ctx->h2, NGHTTP2_FLAG_NONE,
                                 stream->id, &pri_spec);

Otherwise it's unused.

I believe replacing:

 struct stream_ctx *stream = H2_STREAM_CTX(data);

with

int32_t stream_id = H2_STREAM_ID(data);

and then using stream_id directly instead of stream->id will fix the issue.

Testing it now.

@jbgoog
Copy link
Author

jbgoog commented Jun 23, 2023

Here's the stack trace:

(http2.c:1675)		h2_progress_egress
(http2.c:1670)		h2_progress_egress
(http2.c:2199)		cf_h2_connect
(http2.c:2559)		Curl_http2_switch_at
(cf-https-connect.c:182)		baller_connected
(cfilters.c:381)		Curl_conn_connect
(multi.c:2109)		multi_runsingle
(multi.c:2745)		curl_multi_perform

it's unclear if the line numbers are correct but I was able to reproduce the issue with a debugger.

@jbgoog jbgoog mentioned this issue Jun 23, 2023
Closed
@jbgoog
Copy link
Author

jbgoog commented Jun 23, 2023

This appears to fix the issue: #11380

🥳

@icing
Copy link
Contributor

icing commented Jun 24, 2023

Thanks. While this avoids the crash, it does not do the needed behaviour. This submits the priority to nghttp2 on stream id -2 and not the stream that, afterwards, will be opened in nghttp2.

The correct way is to delay the priority handling until the stream has been opened. The if should only be entered here when in addition stream != NULL && stream->id > 0. Could you amend your PR in this way?

@icing icing self-assigned this Jun 24, 2023
@icing icing mentioned this issue Jun 26, 2023
Merged
icing added a commit to icing/curl that referenced this issue Jun 26, 2023
@icing
HTTP/2, fix crash in handling stream weights
41e5345 
- refs curl#11379 where the crash was reported
- weights may change "on the run", which is why there are checks
  in general egress handling. These must not trigger when the stream
  has not been opened yet.
- add test2404 to reproduce and verify
@icing
Copy link
Contributor

icing commented Jun 26, 2023

I made #11384 with a test case and fix.

@jbgoog
Copy link
Author

jbgoog commented Jun 26, 2023

Awesome Amazing!

@jay jay closed this as completed in 29f33b3 Jun 29, 2023
markg85 pushed a commit to markg85/curl that referenced this issue Jun 29, 2023
@icing @markg85
http2: fix crash in handling stream weights
f06b8da 
- Delay the priority handling until the stream has been opened.

- Add test2404 to reproduce and verify.

Weights may change "on the run", which is why there are checks in
general egress handling. These must not trigger when the stream has not
been opened yet.

Reported-by: jbgoog@users.noreply.github.com

Fixes curl#11379
Closes curl#11384
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
@icing @bch
http2: fix crash in handling stream weights
cd45dae 
- Delay the priority handling until the stream has been opened.

- Add test2404 to reproduce and verify.

Weights may change "on the run", which is why there are checks in
general egress handling. These must not trigger when the stream has not
been opened yet.

Reported-by: jbgoog@users.noreply.github.com

Fixes curl#11379
Closes curl#11384
ptitSeb pushed a commit to wasix-org/curl that referenced this issue Sep 25, 2023
@icing @ptitSeb
http2: fix crash in handling stream weights
fff2eaf 
- Delay the priority handling until the stream has been opened.

- Add test2404 to reproduce and verify.

Weights may change "on the run", which is why there are checks in
general egress handling. These must not trigger when the stream has not
been opened yet.

Reported-by: jbgoog@users.noreply.github.com

Fixes curl#11379
Closes curl#11384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@icing icing

Labels
crash HTTP/2
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replaced stream->id with H2_STREAM_ID macro jbgoog/curl
4 participants
@icing @dfandrich @jay @jbgoog