bugGNU Wget - Bugs: bug #20653, Heuristic for determining non-HTML...

 
 

bug #20653: Heuristic for determining non-HTML files

Submitted by:  Micah Cowan <micahcowan>
Submitted on:  Wed 01 Aug 2007 06:38:25 PM UTC  
 
Category: Feature RequestSeverity: 3 - Normal
Priority: 9 - ImmediateStatus: Confirmed
Privacy: PublicAssigned to: Derek T. Doyle <derektdoyle>
Originator Name: Open/Closed: Open
Release: NoneOperating System: None
Reproducibility: NoneFixed Release: None
Planned Release: 1.14Regression: None
Work Required: 1 - DaysPatch Included: Yes

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sun 14 Dec 2008 10:26:30 PM UTC, comment #8:

Here's a patch that does what I'm suggesting.

Thanks.

(file #17061)

Derek T. Doyle <derektdoyle>In charge of this item.
Sun 14 Dec 2008 12:35:03 AM UTC, comment #7:

You're quite welcome. I find Wget very useful both professionally and personally, so it's nice to be able to help out. Thank you for all your hard work on it.

Good points all around. I've added p, div, and span to the list along with table, form and li. I corrected the strncpy issue by only reading just the length of fm->content if it's less than 1024, 1024 if it's greater or equal, and always terminate it by placing \0 one character beyond whatever the read length turned out to be. Naturally, fk is now 1025 long instead of 1024, to make room for a possible \0 at that position.

It is now clear that c_tolower() isn't going to work the way I was using it. As a result of your suggestion, I looked at strcasecmp(), but it only compares entire strings. There doesn't seem to be a strcasestr() or stristr() in use anywhere to search for a substring case-insensitively. In the spirit of your suggestion about insensitively matching DOCTYPE, I'm considering simply iterating through the length of the first 1k (or however much) and lower casing it all using c_tolower, then only comparing against the lowercase version of all the tags using strstr(). This seems like the least-code, permissive-but-not-too-permissive recognition scheme, letting in
"<hTml" and "<!DoctypE html" rare and out-of-spec though they may be. It would also allow our struct to fall back to a simple array, discarding the case_sensitive flag. What would you think of this approach?

As far as "<p ", "<p>", and "<p/>", my instinct is to try each of these endings on all the tags in rec_tag_list as a simple rule-of-thumb with only a couple of unlikely cases in the bunch.

Thanks for your input on this.

Derek T. Doyle <derektdoyle>In charge of this item.
Mon 01 Dec 2008 10:41:44 PM UTC, comment #6:

Yeah, although now it'll also identify non-HTML SGML or XML applications as "HTML". But it is just a heuristic, after all.

I suspect that putting it back to "<!DOCTYPE HTML" and making it case-insensitive might produce mildly more-accurate results (even though "DOCTYPE" should never appear in lowercase).

Since we want this heuristic to err on the side of false positives rather than false negatives, it's probably worthwhile to add recognition for tags such as p, div, span, and possibly a few others, as folks have been known to omit head/body tags from HTML files. "<p" is probably too permissive, as it matches "<polyester"; but perhaps "<p ", "<p>", and "<p/>" are good.

As I read the code, that strncpy() will fail to result in a null-terminated string in the (likely) event that the length of the file's contents exceeds 1024. This makes it a bad thing to pass to strstr. (Also, if the content length is much less than 1024 (say, 80), strncpy() will still write a total of 1024 bytes.)

The use of c_tolower() to handle "case-insensitive" matching isn't going to work properly: it converts a single character, whereas you're passing a string. Bad things may happen. :)... anyway, it's not going to catch silly things like "<hTml"; better to use strcasecmp() instead.

BTW, thanks very much for helping with this!

Micah Cowan <micahcowan>
Sun 30 Nov 2008 05:15:20 PM UTC, comment #5:

Right.

I trimmed doctype check back to just "<!DOCTYPE" and made it case sensitive, restricted to upper case.

The loop over an array of specially-written structs sounded correct and elegant, so here it is in a patch that replaces the first one.

Thanks.

(file #16951)

Derek T. Doyle <derektdoyle>In charge of this item.
Mon 24 Nov 2008 02:58:35 AM UTC, comment #4:

Of course, <!DOCTYPE HTML is also permissible (but not, as you know, <!doctype HTML).

Could probably implement the series of "if (tag_recognized" as a loop over an array of specially-written structs (string/case-sensitivity pair)?

Micah Cowan <micahcowan>
Mon 24 Nov 2008 12:01:39 AM UTC, comment #3:

OK.

I have submitted a patch that seems to work, in line with what I described in comment #2.

Currently it recognizes for DOCTYPE, html, head, and body (for the last three both upper and lower case). Other tags could easily be added.

Thanks.

(file #16909)

Derek T. Doyle <derektdoyle>In charge of this item.
Sat 22 Nov 2008 04:25:51 PM UTC, comment #2:

Hi,

This feature looks like an interesting one to add. I have an approach in mind that I think would make this happen. Could I get this feature assigned to me, please?

The place to make it happen seems to be in get_urls_html() in html-url.c, where fm->content is available to provide the first 1k of the file just prior to where map_html_tags() is called to do the parsing. I figure that's where a simple check can be done for a few common tags.

If this heuristic would be useful anywhere else in the code, it could easily be made into a separate function.

Thanks.

Derek T. Doyle <derektdoyle>In charge of this item.
Wed 01 Aug 2007 06:39:37 PM UTC, comment #1:

This request was filed in response to bug 20496.

Micah Cowan <micahcowan>
Wed 01 Aug 2007 06:38:25 PM UTC, original submission:

It could be useful for some heuristic mechanism to refuse to parse files that do not appear to be HTML. The mechanism would need to err on the safe, "is HTML" side, especially since many encodings could look a lot like binary data. Perhaps just checking for the existence of some recognized tag within the first 1k or so...

Micah Cowan <micahcowan>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #16951:  wget_fix_for_bug_20653.2.patch added by derektdoyle (3KiB - text/x-patch - Here is the refinement as suggested by Micah, which replaces the previous patch.)
file #16909:  wget_fix_for_bug_20653.patch added by derektdoyle (3KiB - text/x-patch - Here is the first attempt, which seems to work.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by derektdoyle (Posted a comment)
  • -unavailable- added by derektdoyle (Looks interesting.)
  • -unavailable- added by micahcowan (Submitted the item)
  •  

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 14 Dec 2008 10:26:30 PM UTCderektdoyleAttached File-=>Added wget_fix_for_bug_20653.3.patch, #17061
    Mon 01 Dec 2008 10:41:44 PM UTCmicahcowanPriority4=>9 - Immediate
      Patch IncludedNone=>Yes
    Sun 30 Nov 2008 05:15:20 PM UTCderektdoyleAttached File-=>Added wget_fix_for_bug_20653.2.patch, #16951
    Mon 24 Nov 2008 02:58:35 AM UTCmicahcowanPlanned Release1.15=>1.14
    Mon 24 Nov 2008 12:01:39 AM UTCderektdoyleAttached File-=>Added wget_fix_for_bug_20653.patch, #16909
    Sat 22 Nov 2008 09:42:40 PM UTCmicahcowanAssigned toNone=>derektdoyle
    Sat 22 Nov 2008 04:18:13 PM UTCderektdoyleCarbon-Copy-=>Added -unavailable-
    Sun 27 Jan 2008 04:27:43 AM UTCmicahcowanPlanned Release1.12=>1.15
    Thu 09 Aug 2007 09:39:40 PM UTCmicahcowanWork RequiredNone=>1 - Days
    Wed 01 Aug 2007 06:39:37 PM UTCmicahcowanPriority5 - Normal=>4

    Back to the top


    Powered by Savane 3.1-cleanup1