GNOME Bugzilla – Bug 647767
Use "%u" as printf format for port number in soup_uri_to_string
Last modified: 2011-04-15 16:14:37 UTC
Detected while running a WebKit test that was showing a -1 instead of a G_MAXUINT. Nobody should have real world issues with that (unless using a 16bit machine) but it does not hurt :).
Created attachment 185951 [details] [review] Patch Adding also a test case. Not sure if it pays off.
Comment on attachment 185951 [details] [review] Patch > Adding also a test case. Not sure if it pays off. The soup-uri.c part is definitely right, and OK to commit. I'm unsure about the test; TCP ports can only be 0-65535, so the test is like "make sure we do the right thing in a case that the caller shouldn't be doing and that shouldn't do anything useful anyway if they do". (The reason the API uses guint rather than gushort is that glib doesn't have G_TYPE_USHORT, so properties like SoupAddress:port have to be a guint, and so I just used guint for ports everywhere else too for consistency.) What WebKit test is this that was failing? (Actually, in general, if you're filing a patch to fix a WebKit test, please specify which WebKit test in the commit message.) (And if you were going to add a test, I'd just add { "http://host:4294967295/", "http://host:4294967295/" } to the end of abs_tests.)
(In reply to comment #2) > (From update of attachment 185951 [details] [review]) > > Adding also a test case. Not sure if it pays off. > > The soup-uri.c part is definitely right, and OK to commit. I'm unsure about the > test; TCP ports can only be 0-65535, so the test is like "make sure we do the > right thing in a case that the caller shouldn't be doing and that shouldn't do > anything useful anyway if they do". (The reason the API uses guint rather than > gushort is that glib doesn't have G_TYPE_USHORT, so properties like > SoupAddress:port have to be a guint, and so I just used guint for ports > everywhere else too for consistency.) Dan, I absolutelly agree with you on that, that's why I said it didn't pay off. > What WebKit test is this that was failing? (Actually, in general, if you're > filing a patch to fix a WebKit test, please specify which WebKit test in the > commit message.) Sure, thing is that the test requires this change and some others in WebKit that are still not ready to be committed, that's why I didn't mention the test, because you wouldn't be able to see the effects of this change without WebKit's. > (And if you were going to add a test, I'd just add > > { "http://host:4294967295/", "http://host:4294967295/" } > > to the end of abs_tests.) I think I'm just committing the %d -> %u part.
Committed 7a15b34ec9510263035cd2af3f9f38d36cfe77db
(In reply to comment #3) > (In reply to comment #2) > > What WebKit test is this that was failing? (Actually, in general, if you're > > filing a patch to fix a WebKit test, please specify which WebKit test in the > > commit message.) > > Sure, thing is that the test requires this change and some others in WebKit > that are still not ready to be committed, that's why I didn't mention the test, > because you wouldn't be able to see the effects of this change without > WebKit's. It's still nice to know what test is involved though, so I can figure out exactly what they're testing / why they think this test case is useful, etc.
(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > > > What WebKit test is this that was failing? (Actually, in general, if you're > > > filing a patch to fix a WebKit test, please specify which WebKit test in the > > > commit message.) > > > > Sure, thing is that the test requires this change and some others in WebKit > > that are still not ready to be committed, that's why I didn't mention the test, > > because you wouldn't be able to see the effects of this change without > > WebKit's. > > It's still nice to know what test is involved though, so I can figure out > exactly what they're testing / why they think this test case is useful, etc. It's security/block-test.html. One of the things it tests is how WebKit behaves with invalid port numbers, that's why it tests 2¹⁶, 2¹⁶ - 1, 2³² and 2³²-1