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.

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

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

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
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 added a commit to icing/curl that referenced this issue Jun 26, 2023
- 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
- 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
- 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
- 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants